-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
Make colormap handling consistent and allow discrete mapping #2483
Conversation
@jbednar @jlstevens Would be good to get your thoughts on this. |
Due to some small changes in colormapping the test data needs updating, as far as I can tell the changes are not visually perceptible, at least on my monitor. |
I'd favor adding colorcet, but I'm not sure my vote should be the deciding factor. |
That is pretty much entirely orthogonal to this PR although I'm happy to add them. At least now they should appear identical in matplotlib and bokeh. What fraction of colorcet colormaps now have readable names btw? You can already use colorcet colormaps with HoloViews by passing them in explicitly all "adding it" means that you can reference them by name, which isn't very useful with obscure names. |
Out of the 51 distinct colormaps in colorcet, 21 have human-friendly names:
Or if you count the |
It's nice to be able to pass them in by name like this, but it's also nice to be sure that you are using perceptually uniform maps, and that's hard to tell from the names (plus inferno, plasma, viridis, and a few other common uniform ones. I guess we could provide a flag people could set "warn_for_perceptually_nonuniform_colormaps"? |
Okay, that definitely seem sufficient to include, at least when colorcet is available. No opinion on requiring it.
That's true, I think documenting them well is the most important thing. I'm not opposed to adding an option to warn but I'm not sure how discoverable it would be in practice. |
Here is the full list including named colorcet maps, some are duplicated because they have different names in bokeh and matplotlib. I've tried to smooth over those differences as much as possible, i.e. if you request the matplotlib/bokeh name and only the other is installed it'll treat it as an alias. |
8735c43
to
38e690e
Compare
Perhaps more helpful if split up into three sections: MatplotlibBokehColorcetIf there is a clash it will always pick matplotlib, then bokeh, then colorcet. The other rules are:
|
holoviews/plotting/mpl/element.py
Outdated
|
||
if isinstance(cmap, dict): | ||
factors = np.unique(values) | ||
palette = [cmap.get(f, colors.get('NaN', {'color': '#8b8b8b'})['color']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this magic value '#8b8b8b'? I'm guessing it is some grayish color...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, can probably specify that somewhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a default_nan_color
or color_for_missing_values
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really color_for_values_which_have_not_been_assigned_an_explicit_color
.
Splitting it up is definitely important, and it could be useful to split it by backend, but I would think that a categorical distinction would be much more useful (linear (aka sequential), grey, diverging, cyclic, qualitative, isoluminant) the way matplotlib and colorcet do. If you give me your code for rendering the colorbars above, I'd be happy to categorize all the colormaps in this way, using https://matplotlib.org/tutorials/colors/colormaps.html and colorcet's naming scheme to establish the categories. And then if there were a way to mark which ones are perceptually uniform, I think this could be a very good reference for helping people choose colormaps. |
This split was for illustration purposes on this PR and for internal usage, not for documentation. I'd be happy to list them separately. If you're willing to take that on I'd suggest a second argument to the new |
holoviews/plotting/bokeh/element.py
Outdated
@@ -1046,7 +1046,8 @@ def _get_colormapper(self, dim, element, ranges, style, factors=None, colors=Non | |||
low, high = None, None | |||
|
|||
cmap = colors or style.pop('cmap', 'viridis') | |||
nan_colors = {k: rgba_tuple(v) for k, v in self.clipping_colors.items()} | |||
nan_colors = {k: (0, 0, 0, 0) if v == 'transparent' else rgba_tuple(v) | |||
for k, v in self.clipping_colors.items()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't just insert 'transparent' into some more general location not having to do with clipping_colors, so that 'transparent' is always a valid color value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There ought to be an HTML color that means this imho!
Sadly, I don't think there is one though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the check to rgba_tuple instead won't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, can do, I could also add it to the MPL_COLORS
dictionary and perhaps name it something more generic like COLOR_ALIASES
. Currently it just defines the matplotlib color shorthands b, c, g, k, m, r, w, y.
@jbednar Would you like to do the colormap categorization in this PR or do you want to follow up in another later? |
From my perspective this is now ready to review, but @jbednar will have to say if he wants to list the colormaps in groups in this PR. |
Although I suppose we could also extend the color cycle as suggested in #1591 in this PR. The final choice was:
|
I can make another pr; this one can be merged. |
d0b05ec
to
a66dac5
Compare
d0e75c2
to
7de20cb
Compare
Okay, had to resolve some final issues with mpl and bokeh colormap consistency but I've now added unit tests so this is definitely ready for review. |
I'm editing the Styling Plots notebook now, so please do review but probably best to hold off from merging until I'm done. |
Edits look good to me, over to @jlstevens for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes all look good to me in the latest version, apart from wondering why clipping_colors is defined in so many separate places.
I'm done making changes except that I'll still try to pull out the colormaps into different categories in the styling notebook. Merging doesn't have to wait on that, though; it should be a minor and very local change. |
Mainly because it's appropriate for raster-like types but not anything else, e.g. mapping points, vectors to be transparent is obviously not correct, so we can't just declare it on the baseclass. |
Ok. Looks like fixing the four different places it's currently redefined would require a new base class RasterlikePlot under ColorbarPlot, and I guess it's not worth that. |
Right, and we'd either have to define a mixin class or separate subclasses for each backend. |
Looks good to me. Merging. |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR makes colormapping more consistent between backends:
color_levels
to generate discrete colormaps ( Use discrete colorbars for contours and polygons #1856)MPL_COLORS
in holoviews (Make matplotlib a soft dependency of the bokeh backend #829, plotting/bokeh/util.py depends on matplotlib even for bokeh backend #1807)Styling_Plots
user guide which currently covers Cycles/Palettes and colormaps.This means that the following list of colormaps are now supported consistently across backends:
I'm not opposed to adding colorcet to that list, but it's now no longer required for our colormap handling to be consistent.