ENH: Add cmasher.combine_cmaps

[DanielYang59]

Overview

Add a custom function to easily combine multiple matplotlib colormaps at given positions:

# Example 1: Using colormap names
cmaps = ["ocean", "prism", "coolwarm"]
nodes = [0.2, 0.75]
custom_cmap_1 = combine_cmaps(cmaps, nodes)

# Example 2: Using predefined colormaps
cmap_0 = plt.get_cmap("Blues")
cmap_1 = plt.get_cmap("Oranges")
cmap_2 = plt.get_cmap("Greens")
custom_cmap_2 = combine_cmaps(cmap_0, cmap_1, cmap_2)

EDIT by @neutrinoceros : I've updated the illustrative example to use the final version of the interface for posterity. The original example is still viewable in the following. The only difference is on the last line.

Orignal example

# Example 1: Using colormap names
cmaps = ["ocean", "prism", "coolwarm"]
nodes = [0.2, 0.75]
custom_cmap_1 = combine_cmaps(cmaps, nodes)

# Example 2: Using predefined colormaps
cmap_0 = plt.get_cmap("Blues")
cmap_1 = plt.get_cmap("Oranges")
cmap_2 = plt.get_cmap("Greens")
custom_cmap_2 = combine_cmaps([cmap_0, cmap_1, cmap_2])

TODO list

• Add function
• Add unit tests

[DanielYang59]

Here I enclose a minimal code snippet to test this function:

cmaps = ["Blues", "Oranges", "Greens"]
nodes = [0.2, 0.75]
custom_cmap = combine_cmaps(cmaps, nodes)

# Test plotting with the custom colormaps
x = np.linspace(0, 10, 100)
y = np.sin(x)

# Example plot
fig, ax = plt.subplots()
sc = ax.scatter(x, y, c=x, cmap=custom_cmap, marker='o')
plt.colorbar(sc, ax=ax, label='Sample Data')

plt.show()

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a68d6ee) 98.36% compared to head (4f06d03) 98.52%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #122      +/-   ##
==========================================
+ Coverage   98.36%   98.52%   +0.16%     
==========================================
  Files           6        6              
  Lines         611      676      +65     
==========================================
+ Hits          601      666      +65     
  Misses         10       10              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[neutrinoceros]

Hi @DanielYang59, thank you for your work ! This seems like a reasonable addition to the package, but before I can do an in-depth review, please address the following points:

• stylistic changes in unrelated lines should be reverted (let's keep the scope of the PR as narrowly defined as possible). If you wish, please open a separate PR for these.
• there are some type-checking errors, please address them. Let me know if you need a hand with this part.

Thanks !

[neutrinoceros]

Actually, and I'm sorry if this sounds distressing, but I need to ask: what's your reasoning for adding this function to cmasher, specifically ? it doesn't seem to rely on existing cmasher functionality, and, as you are well aware it is perfectly fine outside the package.
Cmasher is primarily a colormap library and happens to have a couple public utility functions, all of which primarily exist to serve the package own needs, and I am not sure we should set a precedent by accepting a patch that deviates from this scope.
I don't mean to kill your spirit: it's really nice of you to propose this ! Though, as a maintainer I need to be careful not to overload my own burden, which sometimes means saying no.

Have you considered contributing this to other matplotlib extension packages ? https://matplotlib.org/mpl-third-party/

[DanielYang59]

Actually, and I'm sorry if this sounds distressing, but I need to ask: what's your reasoning for adding this function to cmasher, specifically ? it doesn't seem to rely on existing cmasher functionality, and, as you are well aware it is perfectly fine outside the package. Cmasher is primarily a colormap library and happens to have a couple public utility functions, all of which primarily exist to serve the package own needs, and I am not sure we should set a precedent by accepting a patch that deviates from this scope. I don't mean to kill your spirit: it's really nice of you to propose this ! Though, as a maintainer I need to be careful not to overload my own burden, which sometimes means saying no.

Thanks a lot for asking here. The reason actually went through only limited mind work... Just me happening to find cmasher to be a somewhat colormap centered package and has a get_sub_cmap function in utils, so I thought maybe that is the place my function should go? Thus this PR. I fully understand your concern and I'm more than happy to move this to more suitable packages if you decide somewhere else suits it better (I looked into the mpl third party tools but doesn't find any other package more suitable than cmasher, do you have any suggestions? Thanks!).

[neutrinoceros]

Just me happening to find cmasher to be a somewhat colormap centered package and has a get_sub_cmap function in utils, so I thought maybe that is the place my function should go?

that's reasonable, but I'll ask @1313e to make the final call here :)

[DanielYang59]

that's reasonable, but I'll ask @1313e to make the final call here :)

Sure thanks for that.

Meanwhile I'm getting another error from ruff-format for some reason:

ruff-format..............................................................Failed
- hook id: ruff-format
- files were modified by this hook

What might be the reason for this? Thanks.

[neutrinoceros]

What might be the reason for this? Thanks.

I suggest to install and run pre-commit locally. You can also use the magic comment pre-commit.ci autofix as the report suggests.

[DanielYang59]

pre-commit.ci autofix

[DanielYang59]

suggest to install and run pre-commit locally. You can also use the magic comment pre-commit.ci autofix as the report suggests.

Thanks a ton! Another new trick learned. However I'm getting some trouble with my local SSL module and I'll fix it ASAP. Thanks a lot for your patience!

[neutrinoceros]

Now that CI is all green, here's an in-depth review !

[neutrinoceros]

I'm not sure this is helpful: colormaps don't have an absolute orientation, and may be displayed horizontally. Is there a way to rephrase this note that would work independently of orientation ?

[DanielYang59]

I was thinking the same about the necessity of having the note and the phrasing of the note yesterday.

As the LinearSegmentedColormap.from_list() function would combine the cmaps from lower value end to higher value end (if not reversed, that is bottom to top). So this is expected behaviour for the cmaps to start from the lower value end. But meanwhile for me, I expected the cmaps to be combined from top to bottom at first attempt, when I don't have much understanding of Colormap.

Maybe we could remove this note altogether? As the behaviour is expected?

Or rephrase it to something like "The colormaps are combined from low value to high value end."? What would your suggestion be?

[1313e]

that's reasonable, but I'll ask @1313e to make the final call here :)

Yeah, I think it can have a place in CMasher.
Having said that, any utility function that is added also needs to have its own section in the online docs, explaining what the purpose of said utility function is with examples.

[DanielYang59]

Yeah, I think it can have a place in CMasher. Having said that, any utility function that is added also needs to have its own section in the online docs, explaining what the purpose of said utility function is with examples.

Thanks for granting the permission 😄! Sure I would work with @neutrinoceros to draft a docs as well. Thanks a ton.

[neutrinoceros]

This round of review is mostly about the interface design, as I think it should be converged before you write documentation. Thank you !

[neutrinoceros]

In this round of review, I'll make suggestions to change the interface so that positional arguments are used for cmaps instead of a list as the first argument. This is the fundamental change, all following suggestions will be adapting call sites and docs

Suggested change

	cmaps: list[Union[Colormap, str]],
	*,
	*cmaps: Union[Colormap, str],

[DanielYang59]

I think I'm not understanding this change well enough here (especially the pros and cons)...

In current implementation, cmaps would take exactly a list; while in the suggested implementation, any number of positional args would be recognised as parts of cmaps and would be zipped during runtime.

From my understanding, the current implementation is "stricter" by explicitly requiring a list of Colormaps, and suggested implementation being more flexible ("lighter" as you described).

By making this change (and the other change to make all other args keyword), we are being more flexible by requesting other args to be keyword, leaving only cmaps positional, am I right?

I feel this should be safe and good as this utility would only take several Colormaps and combine them, so it's reasonable to expect cmaps to be the focus here.

[neutrinoceros]

For the record, the reason why CI is failing now is that you added an implicit dependency on pandas for tests, and pandas isn't included in our test env. I suggest to simply drop this test case and avoid depending on pandas entirely.

[neutrinoceros]

Oh, and I'm just noticing now, but we don't merge on master directly in this project, I'll change the target branch now. Hopefully this will be 100% transparent to you.

[neutrinoceros]

Really learned a lot during this PR 😊.

I'm glad you're enjoying it ! we're almost there, I think all we need now is documentation as requested by @1313e :)

[DanielYang59]

I'm glad you're enjoying it ! we're almost there, I think all we need now is documentation

More than enjoying this (thanks really a lot for your patience and all these valuable suggestions). Yes actually I had already added a draft section under docs/source/user/usage.rst (hopefully I'm not adding to the wrong place) and would push it once it's done.

[DanielYang59]

Do you have any advice on the docs @neutrinoceros ? Thanks!

[neutrinoceros]

@DanielYang59 I think a new section in docs/source/user/usage.rst would do the trick. Thanks !

[DanielYang59]

@DanielYang59 I think a new section in docs/source/user/usage.rst would do the trick. Thanks !

Thanks @neutrinoceros. I would see if I could pre-render the rst locally for preview, as I totally have no prior experience with reStructuredText and just followed the grammar of other parts in that docs file 😄 .

[neutrinoceros]

building docs locally should boil down to something like

python -m pip install -r requirements/docs.txt
sphinx-build -M html docs/source site -W

[DanielYang59]

building docs locally should boil down to something like

python -m pip install -r requirements/docs.txt
sphinx-build -M html docs/source site -W

Thanks a lot. Could confirm it works for me. This section looks good to me. Do you have any suggestions?

[neutrinoceros]

Looks cool ! I think I would make the example use 3 color maps to make it even clearer that this works with any number of them.

[neutrinoceros]

Also, try to pick a set of colormaps that don't have any color in common so the example also illustrates communication best practices.

[DanielYang59]

Also, try to pick a set of colormaps that don't have any color in common so the example also illustrates communication best practices.

Yes this makes sense. In fact I started with two distinct colormaps ("prism" and "Pastel2") but then noticed other parts of the docs use "rainbow" and "lilac" so just tried to be consistent. I would implement the changes now.

[DanielYang59]

What about now @neutrinoceros ?

Update:
just replaced "prism" with "PuBu" to make it easier to the eyes.

[neutrinoceros]

While trying to find a more realistic combination (something that users would actually do), I found that from cmasher import combine_cmaps doesn't work (it should be re-exported in __init__.py).

After experimenting with it, I find that, actually, the only examples I can come up with that feel natural to me involve only two colormaps, so I changed my mind. How about combine_cmaps("cmr.torch", "cmr.horizon_r") ?

[1313e]

Yes this makes sense. In fact I started with two distinct colormaps ("prism" and "Pastel2") but then noticed other parts of the docs use "rainbow" and "lilac" so just tried to be consistent. I would implement the changes now.

cmr.rainforest is often used by me in the docs as it was the first colormap I ever designed. :)
After it became popular at the university I used to work at, I decided to start making a proper collection of colormaps, which later became CMasher.
So, I have a soft-spot for that colormap specifically. :)

On the docs: They look great and with neutrinoceros' suggestion, I think it will be perfect.
One single comment though: Please refer to CMasher as just "CMasher", not "the CMasher". ;)

[DanielYang59]

cmr.rainforest is often used by me in the docs as it was the first colormap I ever designed. :) After it became popular at the university I used to work at, I decided to start making a proper collection of colormaps, which later became CMasher. So, I have a soft-spot for that colormap specifically. :)

Thanks a lot for sharing this background story, that's awesome (cmr.rainforest looks great).

Please refer to CMasher as just "CMasher", not "the CMasher". ;)

Thanks, this has been rectified 😄 .

[DanielYang59]

While trying to find a more realistic combination (something that users would actually do), I found that from cmasher import combine_cmaps doesn't work (it should be re-exported in __init__.py).

This has been fixed. Thanks for pointing out.

After experimenting with it, I find that, actually, the only examples I can come up with that feel natural to me involve only two colormaps, so I changed my mind. How about combine_cmaps("cmr.torch", "cmr.horizon_r") ?

I guess this is really hard decision here. If we use two distinct colormaps (like my case), it might be easier for docs reader to tell one colormap from another, but feels less natural (and looks much less beautiful). On the other hand, if we use two naturally fitting together colormaps, it might be hard to tell the boarder.

What about combine_cmaps("cmr.rainforest", "cmr.torch_r") then (to keep @1313e's lovely cmr.rainforest)?

[neutrinoceros]

rainforest + torch_r looks awesome. And thanks Ellert for sharing, I also didn't know that rainforest was special :)

[DanielYang59]

Should be good now. Please review @neutrinoceros.

[neutrinoceros]

Docs look very good, I just have a couple nitpicks ! While I'm here, is it okay with you if I just squash your branch before I merge ? I don't think we need to keep all original 50 commits

[DanielYang59]

is it okay with you if I just squash your branch before I merge ?

Yes sure, please do that @neutrinoceros . Thanks a lot!

[DanielYang59]

Thanks a lot @neutrinoceros @1313e , enjoyed this process.

[neutrinoceros]

Thank you for going through this with us !
There's been a hiccup in the release process so it might not land on PyPI + Conda-forge today as I hoped, but we'll be working on it !

[DanielYang59]

I noticed that too:

installing twine...
Uploading distributions to https://upload.pypi.org/legacy/
Uploading cmasher-1.8.0-py3-none-any.whl
25l
  0% ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 0.0/546.0 kB • --:-- • ?
 33% ━━━━━━━━━━━━━╺━━━━━━━━━━━━━━━━━━━━━━━━━━ 180.2/546.0 kB • 00:01 • 9.5 MB/s
100% ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 546.0/546.0 kB • 00:00 • 8.4 MB/s
100% ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 546.0/546.0 kB • 00:00 • 8.4 MB/s
25hWARNING  Error during upload. Retry with the --verbose option for more details. 
ERROR    HTTPError: 403 Forbidden from https://upload.pypi.org/legacy/          
         Invalid or non-existent authentication information. See                
         https://pypi.org/help/#invalid-auth for more information.              
Error: Process completed with exit code 1.

Take your time though. Thanks for all those suggestions. Learned a lot about details and quality of coding during this PR.