Skip to content

Add a helper to copy a colormap and set its extreme colors. #14645

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

Merged
merged 1 commit into from
Oct 22, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jun 28, 2019

See changelog. See also the number of explicit copies in the examples,
which suggest that the old setter-based API is a bit of a footgun (as
forgetting to copy seems easy).

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@phobson
Copy link
Member

phobson commented Jun 28, 2019

I like this addition and would find it very useful.

I wonder if get_cmap should also return a copy by default. I can imagine that would sufficiently API-breaking that we need to think hard about it. But a get_cmap function that accepts these params as well would be quite nice.

@ImportanceOfBeingErnest
Copy link
Member

Alternatively one could argue that get_cmap() gets you an existing colormap, and e.g. something like new_colormap() gets you a copy of an existing one. This would allow the notably useful API to be incorporated without backwards compatibility issues,

 plt.cm.new_colormap("viridis", bad="white")

@anntzer
Copy link
Contributor Author

anntzer commented Jun 29, 2019

Initially I thought that changing get_cmap to return a copy would be too disruptive, but let's look at the various behavriors of get_cmap:

  • If it is called with a second argument N, it's already returning a new (resampled) colormap anyways; so we don't even need to bother with a copy.
  • If it is called with a string argument (and no N), it is returning a reference to one of the builtin colormaps. I would argue that these are intended to be immutable and that modifying them should be considered a bug. (Although I would readily admit that one can get away with modifying them in single use scripts.)
  • If it is called with a colormap argument, it returns that colormap (without ever resampling it). If the call-site controls the argument, one may wonder why they additionally pass it through get_cmap; if the argument comes from "outside", then it should likely be considered immutable as well.

Thus, I think modifying get_cmap to return a copy is more likely to fix accidental modifications of builtin or custom cmaps rather than be disruptive.

On the other hand, I would say that get_cmap(...).with_extremes(bad=...) (or new_cmap(...).with_extremes(...) -- throughout this paragraph, read whatever spelling you prefer) is not that worse than get_cmap(..., bad=...), and that with_extremes has the advantage of also working with non-builtin cmaps (and having the functionality both in get_cmap and in with_extremes means having to duplicate the code and the docs, yada yada). Finally, having the parameters in get_cmap also modify a non-builtin-colormap passed as argument would be inconsistent with the N parameter of get_cmap, which, for the better or (mostly?) the worst, does not resample non-builtin cmaps.

@timhoffm
Copy link
Member

As you've pointed out get_cmap has quite some odd behavior. I think it should just return a registered colormap and that should be immutable. No passing through of Colormap inputs, no resampling etc. everything else should be done by creating new colormaps from existing colormap objects.

Since Colormaps are currently not immutable, get_cmap should return copies of the internal colormaps. It then makes sense to have Colormap.resampled() and Colormap.with_extremes(). Maybe even better to have just one function Colormap.copy(N=None, bad=None, under=None, over=None) - naming is open for better suggestions.

Side remark: I'm also not sure if the segmentation N should be a is a fundamental aspect of all colormaps. E.g. one could imagine colormapping by functions. OTOH maybe there is just no need for this and you can always approximate the function on N segments.

See changelog.  See also the number of explicit copies in the examples,
which suggest that the old setter-based API is a bit of a footgun (as
forgetting to copy seems easy).
@tacaswell
Copy link
Member

I really like this pattern.

@jklymak
Copy link
Member

jklymak commented Oct 21, 2020

This does seem useful. Wasn't sure how it fit in w/ @timhoffm plans for the colormap registry...

@timhoffm
Copy link
Member

This does not interfere with my plans for the colormap registry (#18503). The registry only conerns registered colormaps. You can still have separate colormap objects and create new ones. In fact, with_extremes() would still be the canonical way to use custom extremes on a registered colormap, e.g. (API not finalized):

my_cmap = mpl.colormaps['jet'].with_extremes(...)

@@ -0,0 +1,13 @@
`.Colormap.set_extremes` and `.Colormap.with_extremes`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've decided not to put links in section titles for aestethic reasons.

But I'm going to merge as is. I'll anyway do a small colormap-related cleanup PR and will update there. While that's a bit of a messy way of working it saves us an extra review roundtrip here.

@timhoffm timhoffm merged commit e89114e into matplotlib:master Oct 22, 2020
@timhoffm timhoffm mentioned this pull request Oct 22, 2020
@anntzer anntzer deleted the with_extremes branch October 22, 2020 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants