Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds functionality for inverting colormap lightness #106

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

andrew-s28
Copy link
Contributor

I went with the option of adding the function invert_lightness to cmocean.tools, since having more than just an _r subscript on colormaps would be confusing.

This solution does require adding colorspacious as a dependency to cmocean.

Closes #105

Here is an example of the inverted lightness colormaps:
invert_lightness_example

@kthyng
Copy link
Contributor

kthyng commented Feb 21, 2024

I see this! Hope to get to it today. 🌈

@kthyng
Copy link
Contributor

kthyng commented Feb 21, 2024

🌈 as a happy symbol of course, not as an implicit endorsement of using rainbow colormaps.

@kthyng
Copy link
Contributor

kthyng commented Feb 21, 2024

@andrew-s28 I realize now I didn't give feedback on this in your original issue, but I'm interested in having the "_i" or "_inverted" access to these colormaps. How hard would this be to implement, and would it take much time to calculate on the fly if a user called it? I think it would be much more user-friendly since I doubt that many users of cmocean use the tools at all. Though I do appreciate your concern it might be confusing to have more underscore-based names, especially since I don't know of any more conventions besides "_r".

@andrew-s28
Copy link
Contributor Author

andrew-s28 commented Feb 21, 2024

@kthyng Thanks for the review!

It would be straightforward to add access to the inverted colormaps via an _i appendix instead of the way it is now. I could implement it such that order of the appendix does not matter (whether _r or _i comes first). There is very little overhead to calling the inversion on the fly, so I think it would be fine to implement it such that whatever colormap the user calls is passed to the inverting function when they call it.

An alternative would be to include the inverted colormaps into -rgb.txt files, similar to how the existing colormaps are implemented. This option does have the advantage that the inverted colormaps would be generated ahead of time and thus would not require the addition of the colorspacious dependency.

Let me know which way you think would be best.

@kthyng
Copy link
Contributor

kthyng commented Feb 21, 2024

An alternative would be to include the inverted colormaps into -rgb.txt files, similar to how the existing colormaps are implemented

Ah, yes, people always like to limit the dependencies. Also many other packages have included the rgb files directly so this would have the advantage of being able to pass them forward if anyone is interested. I would lean this way even if it seems a bit more brute force. What do you think?

@andrew-s28
Copy link
Contributor Author

I admit to being one of those people - I tend to be wary of suggesting additional dependencies, especially to a well-established package and since colorspacious doesn't seem particularly well maintained these days.

I actually prefer the brute force solution myself as well, it also prevents any future maintenance issues with the additional function in cmocean.tools. My only hesitation was the extra _i appendix that could be confusing, but since you don't think that will be a problem, I can go ahead and put together the -rgb.txt files for the inverted colormaps and submit a new PR that allows them to be called directly from cmocean.cm.

@kthyng
Copy link
Contributor

kthyng commented Feb 22, 2024

@andrew-s28 Yes I think that with some good documentation (is that in this same repo? it's been awhile since I updated that and I wrote all this a long time ago) and also maybe multiple aliases this will be clear enough. What about making available both or all *_inv or *_invert and *_i, whatever seems like it would capture a user's tendencies?

@andrew-s28
Copy link
Contributor Author

@kthyng Seems like a reasonable plan to me, I think I will implement it such that if a _i is present in the colormap then it will return an inverted colormap (this would cover all of the potential cases such as _inv and _inverted as well). Should be able to get that done early next week, since everything is already set up to produce the -rbg.txt files.

The documentation is in this repo, so I will add a short description of inverted colormaps to the docs with this PR as well.

- updates plots.py so that example plots are unchanged by additions
- adds documentation for inverted cmaps
@andrew-s28
Copy link
Contributor Author

@kthyng I have updated the PR to used hard-coded rgb files for the inverted colormaps, accessible with a *_i appendix in the same way current colormaps are. Reversed and inverted colormaps are available with a *_r_i or *_i_r appendix (order doesn't matter).

Documentation is added right after the documentation for reversed colormaps.

I also made a few changes to the cmocean.plots module so that plotting functions such as plots.plot_lightness() and plots.plot_gallery() are unchanged with the extra entries in cmap_d.

@kthyng
Copy link
Contributor

kthyng commented Apr 1, 2024

@andrew-s28 Ack sorry this came in at a particularly busy time and then moved down the email list. I'll get to it today or tomorrow I think.

@kthyng
Copy link
Contributor

kthyng commented Apr 5, 2024

@andrew-s28 This looks really great!! I'm going to accept and then will process it on through.

@kthyng kthyng merged commit 05a6a9f into matplotlib:main Apr 5, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add inverting functionality?
2 participants