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

Make colormap handling consistent and allow discrete mapping #2483

Merged
merged 25 commits into from
Mar 29, 2018

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Mar 25, 2018

This PR makes colormapping more consistent between backends:

This means that the following list of colormaps are now supported consistently across backends:

['Blues',
 'BrBG',
 'BuGn',
 'BuPu',
 'GnBu',
 'Greens',
 'Greys',
 'OrRd',
 'Oranges',
 'PRGn',
 'PiYG',
 'PuBu',
 'PuBuGn',
 'PuOr',
 'PuRd',
 'Purples',
 'RdBu',
 'RdGy',
 'RdPu',
 'RdYlBu',
 'RdYlGn',
 'Reds',
 'Spectral',
 'YlGn',
 'YlGnBu',
 'YlOrBr',
 'YlOrRd',
 'Accent',
 'Dark2',
 'Paired',
 'Pastel1',
 'Pastel2',
 'Set1',
 'Set2',
 'Set3',
 'magma',
 'inferno',
 'plasma',
 'viridis']

I'm not opposed to adding colorcet to that list, but it's now no longer required for our colormap handling to be consistent.

  • Add unit tests

@philippjfr philippjfr added the type: feature A major new feature label Mar 25, 2018
@philippjfr
Copy link
Member Author

@jbednar @jlstevens Would be good to get your thoughts on this.

@philippjfr philippjfr added tag: backend: bokeh tag: backend: mpl type: enhancement Minor feature or improvement to an existing feature labels Mar 25, 2018
@philippjfr
Copy link
Member Author

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.

@jbednar
Copy link
Member

jbednar commented Mar 25, 2018

I'd favor adding colorcet, but I'm not sure my vote should be the deciding factor.

@philippjfr
Copy link
Member Author

I'd favor adding colorcet

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.

@jbednar
Copy link
Member

jbednar commented Mar 26, 2018

Out of the 51 distinct colormaps in colorcet, 21 have human-friendly names:

bgy
bgyw
bjy
bkr
bky
blues
bmw
bmy
colorwheel
coolwarm
dimgray
fire
gray
gwv
isolum
kb
kbc
kg
kgy
kr
rainbow

Or if you count the _r variants of each of each one, there are 102 maps and 42 with human-friendly names. Here kbc means black,blue,cyan, bmy means blue,magenta,yellow, etc.

@jbednar
Copy link
Member

jbednar commented Mar 26, 2018

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"?

@philippjfr
Copy link
Member Author

Out of the 51 distinct colormaps in colorcet, 21 have human-friendly names:

Okay, that definitely seem sufficient to include, at least when colorcet is available. No opinion on requiring it.

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.

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.

@philippjfr
Copy link
Member Author

philippjfr commented Mar 27, 2018

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.

screen shot 2018-03-27 at 1 32 51 pm

@philippjfr philippjfr force-pushed the colormap_consistency branch from 8735c43 to 38e690e Compare March 27, 2018 12:37
@philippjfr
Copy link
Member Author

philippjfr commented Mar 27, 2018

Perhaps more helpful if split up into three sections:

Matplotlib

screen shot 2018-03-27 at 1 49 59 pm

Bokeh

screen shot 2018-03-27 at 1 50 35 pm

Colorcet

screen shot 2018-03-27 at 1 50 42 pm

If there is a clash it will always pick matplotlib, then bokeh, then colorcet. The other rules are:

  1. When checking against matplotlib it also checks against the lowercased name
  2. When checking against bokeh it also checks against the capitalized name
  3. 'tab' and 'Category' are treated interchangeably


if isinstance(cmap, dict):
factors = np.unique(values)
palette = [cmap.get(f, colors.get('NaN', {'color': '#8b8b8b'})['color'])
Copy link
Contributor

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...

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

@philippjfr philippjfr Mar 27, 2018

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.

@jbednar
Copy link
Member

jbednar commented Mar 27, 2018

Perhaps more helpful if split up into three sections:

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.

@philippjfr
Copy link
Member Author

philippjfr commented Mar 27, 2018

Splitting it up is definitely important, and it could be useful to split it by backend

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 list_colormaps function, which filters the colormaps by type. You can then update the Styling Plots user guide added in this PR to list each type separately.

@@ -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()}
Copy link
Member

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?

Copy link
Contributor

@jlstevens jlstevens Mar 27, 2018

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...

Copy link
Member

@jbednar jbednar Mar 27, 2018

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?

Copy link
Member Author

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.

@philippjfr
Copy link
Member Author

@jbednar Would you like to do the colormap categorization in this PR or do you want to follow up in another later?

@philippjfr
Copy link
Member Author

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.

@philippjfr
Copy link
Member Author

Although I suppose we could also extend the color cycle as suggested in #1591 in this PR. The final choice was:

colors = ['#30a2da', '#fc4f30', '#e5ae38', '#6d904f', '#8b8b8b', '#17becf',
          '#9467bd', '#d62728', '#1f77b4', '#e377c2', '#8c564b', '#bcbd22']

screen shot 2018-03-28 at 1 52 55 am

@jbednar
Copy link
Member

jbednar commented Mar 28, 2018

I can make another pr; this one can be merged.

@philippjfr philippjfr force-pushed the colormap_consistency branch from d0b05ec to a66dac5 Compare March 28, 2018 03:29
@philippjfr philippjfr force-pushed the colormap_consistency branch from d0e75c2 to 7de20cb Compare March 28, 2018 13:40
@philippjfr
Copy link
Member Author

philippjfr commented Mar 28, 2018

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.

@jbednar
Copy link
Member

jbednar commented Mar 28, 2018

I'm editing the Styling Plots notebook now, so please do review but probably best to hold off from merging until I'm done.

@philippjfr
Copy link
Member Author

Edits look good to me, over to @jlstevens for review.

Copy link
Member

@jbednar jbednar left a 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.

@jbednar
Copy link
Member

jbednar commented Mar 28, 2018

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.

@philippjfr
Copy link
Member Author

The changes all look good to me in the latest version, apart from wondering why clipping_colors is defined in so many separate places.

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.

@jbednar
Copy link
Member

jbednar commented Mar 28, 2018

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.

@philippjfr
Copy link
Member Author

Right, and we'd either have to define a mixin class or separate subclasses for each backend.

@jlstevens
Copy link
Contributor

Looks good to me. Merging.

@jlstevens jlstevens merged commit 3a91c13 into master Mar 29, 2018
@philippjfr philippjfr deleted the colormap_consistency branch March 31, 2018 21:54
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tag: backend: bokeh tag: backend: mpl type: enhancement Minor feature or improvement to an existing feature type: feature A major new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants