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

feat: add Paul Tol's colormaps #48

Merged
merged 4 commits into from
Apr 26, 2024
Merged

Conversation

Jhsmit
Copy link
Contributor

@Jhsmit Jhsmit commented Apr 26, 2024

This PR add Paul Tol's colormaps: https://personal.sron.nl/~pault/
fixes #45

  • I've added the colormaps as tuples of hex color codes, since lists of hex colors weren't accepted. In my testing everything worked fine, however the test_catalog_data test fails because it doesnt expect tuples.
  • The colormap 'incandescent' was described in Paul Tol's blog, but not in the supplied code. I've added it based on the color values in the blog.
  • For the colormap 'rainbow_discrete', there are 23 variations with different amounts of colors. I've added them now with a function which injects them into globals (via `globals().update(...)). If you prefer, I can instead also write a codegen function and hard-code the output.
  • rainbow colormaps are in the 'miscellaneous' category
  • I haven't added any tags, not sure if I should
  • I've manually compared the output of Paul tol's code (which returns matplotlib cmaps) with the added cmaps from cmap, as far as I can tell they are all reproduced correctly.
  • There was no license file included with Paul Tol's code, only the header stating the license is 'Standard 3-clause BSD'. I've added a standard BSD-3 text (the one github generates) and added it in the LICENSE folder, and inserted there the author/date from the python file header.

Some of Paul Tols colormaps also include a color value for 'bad' values (missing, inf, nan), is this something that is supported by cmap? or will be in the future?

@tlambert03
Copy link
Owner

thanks @Jhsmit!
Looks like I had imposed some internal regularity on the format of the catalog data. Not sure there was any particular "need" for that other than relative uniformity. I can convert all the hex codes to 8-bit RGB tuples, unless you have any reservations on that?

@tlambert03
Copy link
Owner

Some of Paul Tols colormaps also include a color value for 'bad' values (missing, inf, nan), is this something that is supported by cmap? or will be in the future?

isn't yet supported, but i see no reason why not. will add a new feature tracking issue

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.55%. Comparing base (14cab08) to head (6eebd65).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #48      +/-   ##
==========================================
+ Coverage   96.49%   96.55%   +0.05%     
==========================================
  Files         151      152       +1     
  Lines        1884     1915      +31     
==========================================
+ Hits         1818     1849      +31     
  Misses         66       66              

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

@tlambert03
Copy link
Owner

i reformatted the data to 8-bit rgb tuples for uniformity with the rest of the catalog (leaving the hex codes as comments). Hope that's ok with you

looking through that pdf you linked in the docstring, I'm reminded how much I like these colormaps. thank you! They look great in the docs too :)

image

@tlambert03
Copy link
Owner

you can preview the new docs here if you want to make sure everything looks good to you:
https://cmap-docs--48.org.readthedocs.build/en/48/catalog/

@tlambert03 tlambert03 changed the title Added Paul Tol's colormaps feat: add Paul Tol's colormaps Apr 26, 2024
@Jhsmit
Copy link
Contributor Author

Jhsmit commented Apr 26, 2024

That looks amazing!

Screenshot_20240426-160401.png

Should we name them rainbow_discrete_01 so they sort better?

@tlambert03
Copy link
Owner

Should we name them rainbow_discrete_01 so they sort better?

hmm, i can probably figure out how to sort better using natural sorting somehow. I think we should name them based on how you suspect people would want to enter them. I think it's probably a bit harder to remember the zero padding rainbow_discrete_01 than it is to just type rainbow_discrete_1... so i suspect it's better to fix the sorting another way

@Jhsmit
Copy link
Contributor Author

Jhsmit commented Apr 26, 2024

There is the natsort python package

@tlambert03
Copy link
Owner

yep. ok for merge here then? And we figure out display sorting in the docs elsewhere?

@Jhsmit
Copy link
Contributor Author

Jhsmit commented Apr 26, 2024

Yes sounds good. There is still the colour map for ground cover mentioned on Paul's web page but it can be added later.

@tlambert03 tlambert03 merged commit 019e667 into tlambert03:main Apr 26, 2024
15 checks passed
@tlambert03 tlambert03 added the enhancement New feature or request label Apr 26, 2024
@tlambert03
Copy link
Owner

one thing I'm noticing is that colorbrewer uses this pattern for discrete/continuous:

colorbrewer:BrBG
colorbrewer:BrBG_3
colorbrewer:BrBG_4
colorbrewer:BrBG_5

so, what would you think of calling them simply:

tol:rainbow_1
tol:rainbow_2
tol:rainbow_3
...

instead of rainbow_discrete_X? This could potentially go for the other names that currently include "discrete" as well. That is, we might just call it tol:nightfall (since those do seem to be mostly categorical colormaps) and a user who wants it interpolated could do Colormap("nightfall", interpolate=True). Alternatively, it could be nightfall and nightfall_9? Though: "9" is definitely harder to remember than "discrete" (so, I'm less concerned about these colormap names than rainbow)

@Jhsmit
Copy link
Contributor Author

Jhsmit commented Apr 29, 2024

Yes, I agree. I named them rainbow_discrete because this was the name in Paul Tol's blog / code.
If we decide to refactor rainbow_discrete, it should probably be refactored to rainbow_whbr since that is the one with the full range of colors

The piece of information that the colormap is discrete is already contained in the flag "interpolation": false / interpolate=True.
Is the mapping between record.json "interpolation": false and Colormap(interpolate=True) 1 to 1?
Discrete color maps always have interpolate=True, right ?

Maybe, and this would probably be a breaking change / refactor, you could consider something like this:

Colormap('tol:rainbow_whbr`, interpolation=True, ncolors=5)
>>> `returns what is now 'rainbow discrete_5' 
Colormap('colorbrewer:accent`, interpolation=True, ncolors=5)
>>> `for qualitative colormaps, without predefined order of colors (such as tol:rainbow), returns the first 5 colors` 
Colormap('bids:fake_parula`, interpolation=True, ncolors=5)
>>> `for sequential colormaps, return 5 colors linearly sampled, ie cmap(norm(np.linspace(0, 1, num=5, endpoint=True))`

The advantage would be you can significantly condense the colormap catalog this way since all the sets of various discrete/qualitative colormaps would merge into one.

However, maybe its confusing that Colormap('tol:rainbow_whbr', interpolation=True, ncolors=5) then does not return a linear sampling of the colormap, but rather something almost like a linear sampling?

@tlambert03
Copy link
Owner

I named them rainbow_discrete because this was the name in Paul Tol's blog / code.

yeah, I noticed that later as well. I do think that's definitely worth strongly considering, so I'm less worried about leaving it as is.

Is the mapping between record.json "interpolation": false and Colormap(interpolate=True) 1 to 1?
Discrete color maps always have interpolate=True, right ?

yep

Colormap('tol:rainbow_whbr, interpolation=True, ncolors=5)`

I think I'd prefer to leave the n in the name, even though it's leads to a bit more verbosity in the catalog. Primarily, because most of the catalog items with a number in them closely mirror the way those names are used in other colormap libraries.


all told, I think i'm fine leaving everything as is. :) thanks for taking the time to weigh in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Addition of Paul Tol's colormaps
2 participants