Changing get_cmap to return copies of the registered colormaps. #16943

greglucas · 2020-03-28T17:29:15Z

This is an alternative proposal compared to making Colormap objects themselves immutable as mentioned in #16296 (comment). I expect this to have some discussion about extensibility and future directions, so I consider this a proposal/idea PR. Previous discussion can be found in issues #16296 and #14645

Note that this isn't really deprecating anything, but it is changing some pretty significant behavior. I wasn't sure if it would need any deprecation/warnings put in somewhere, but figured that could wait until after some discussion as well.

PR Summary

Previously, calling plt.get_cmap() would return a pointer to the object, which would allow you to modify the built-ins unknowingly.

cmap = plt.get_cmap('viridis').set_over('b')

Previously, a second call to get the viridis colormap would return the modified colormap.
cmap == plt.get_cmap('viridis') # True
Now, it will return the original viridis built-in colormap.
cmap == plt.get_cmap('viridis') # False

Implications

Accessing colormaps through the pyplot interface could still mess people up since it is a single object still.

cmap = plt.cm.viridis
cmap.set_over('b')
cmap == plt.cm.viridis # True
cmap == plt.cm.get_cmap('viridis') # False

This also means that there are two copies of the colormap objects created initially now (one in cmap_d and a second deepcopy that I made for locals() to use with the pyplot interface.
Is there a way to make the plt.cm.viridis object point to the function plt.get_cmap('viridis')? That may be and even nicer touch, but I couldn't think of anything quick off the top of my head.
I had to add an equals method to Colormaps to compare lookup tables rather than object pointers that were done before. This will initialize the lookup table if it wasn't already previously, possibly leading to a little more overhead if people are comparing lots of colormaps.

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

tacaswell · 2020-03-28T17:39:56Z

I less disruptive approach to this would be to add a copy kwarg to get_cmap(). We could start off with it defaulting to False.

I think we need to preserve a way to mutate the global color map is that (for better or worse) we support passing the colormap as a string that we then look up for the user, ex

plt.get_cmap('viridis').set_bad('k')
fig, ax = plt.subplots()
ax.imshow(data, cmap='viridis')

If get_cmap always returned a copy that would be impossible.

anntzer · 2020-03-28T18:46:04Z

If get_cmap always returned a copy that would be impossible.

I think that would be a good thing.

tacaswell · 2020-03-28T18:57:05Z

I think that would be a good thing.

Fair, but that is too big of a change to make without lots (and lots) of warning a lead time. In the scale of things to break vs the gain for breaking them vs the potential cost to users of breaking them I don't think this one is worth it.

anntzer · 2020-03-28T19:16:27Z

I think this is the kind of changes that can just get a standard deprecation warning (after something like #14645 goes in so that there's a reasonably simple alternative available) and we can even give it an extra release cycle of to give people more time to adapt, and we can always back out the change if too many people complain.
It's not as if this was some vague and obscure misfeature; in my view modifying the global cmap instance is a pretty big foot cannon.

greglucas · 2020-03-28T21:01:16Z

I think we are all on the same page that this is what should happen and just need to figure out the best path to get there.

The issue I see with adding a keyword argument is that it isn't really helping anyone to move in the new direction. How would people learn that they should set the keyword to True and why would I want to use that keyword argument? People who would want to use the argument are probably already copying the objects anyways.

The problem with a warning on everything is that I think the messages would get obnoxious for users who aren't actually modifying the built-ins. Although, now that I think about this a little more, maybe we could add in a _stale attribute in the meantime which is updated to True anytime a set_() is called and then if someone calls get_cmap() and that cmap has an _stale attribute we could warn that a preregistered cmap was modified and that is deprecated functionality in a future release.

My thought is that this is a bug not a feature.

plt.get_cmap('viridis').set_bad('k')
fig, ax = plt.subplots()
ax.imshow(data, cmap='viridis')

If someone wants that functionality they should register the cmap back into the global state. This would be the proper usage of overwriting the built-in if one wants to do that.

plt.register_cmap(cmap=plt.get_cmap('viridis').set_bad('k'))
fig, ax = plt.subplots()
ax.imshow(data, cmap='viridis')

greglucas · 2020-03-29T16:36:17Z

I added in a deprecation warning that I think will work to warn only when being used in the way we want to deprecate.

I wasn't sure how to add in the new state copies without undoing the previous global overriding, so I have left those calls commented in for now where they should go in a future release. I'm not sure how you approach a situation like this? Add a kwarg or new function with a note to remove or something similar?

timhoffm · 2020-03-29T17:29:12Z

@greglucas Thanks, this is a reasonable aproach and the deprecation is consistent.

However, we need the discussion if we want to make colormaps immutable (c.f. #16296 (comment)). That would be an alternative approach. While technically orthogonal, it would lead to other deprecations and to other replacement code. We don't want to do both, and thus need a discussion before going one way or the other.

timhoffm · 2020-03-29T17:37:41Z

I wasn't sure how to add in the new state copies without undoing the previous global overriding, so I have left those calls commented in for now where they should go in a future release. I'm not sure how you approach a situation like this? Add a kwarg or new function with a note to remove or something similar?

We don't have a standard approach for this. Usually, we go through the deprecation warnings and it's rather self-evident what to change when removing them.

jklymak · 2020-03-29T21:46:48Z

doc/api/next_api_changes/behaviour.rst

+Previously, calling ``get_cmap()`` would return
+the built-in Colormap. If you made modifications to that colormap, the
+changes would be propagated in the global state. This function now
+returns a copy of all registered colormaps to keep the built-in


"... copy of the requested colormap..."?

Should this even go into this now if it is a future change and there are not currently any behavior changes?

My thoughts are to actually break these commits up into two separate PRs. One with the deprecation warning/preparation for a future version. Then another one with the proposed copy code in it that could be pushed off for quite a while and discussed for immutability or not.

I personally think the globals should be immutable. If folks are modifying the global to then call again as a named colormap, then I think they were using a misfeature/bug.

I just gave a comment here because I was reading it, and had a quick thought. Please work out w/ Tim Hoffman how to do the rest of the PR, but this seems to be the right direction to be heading.

jklymak · 2020-03-29T21:52:40Z

lib/matplotlib/cm.py

+                        "colormap in this way has been deprecated. "
+                        "Please register the colormap using "
+                        "plt.register_cmap() before requesting "
+                        "the modified colormap.")


I understand this, but only because I read the thread. If I was a user who ran

plt.get_cmap('viridis').set_bad('k') fig, ax = plt.subplots() ax.imshow(data, cmap='viridis')

I'd have no idea what I was supposed to do here. I also don't think we should be encouraging any changing of the global state. I'd far rather tell folks that they should deal with the cmap object which is far less arcane than registering the global state:

cmp = plt.get_cmap('viridis').set_bad('k') fig, ax = plt.subplots() ax.imshow(data, cmap=cmp)

To that end, I'd say "Modifying a registered colormap is deprecated; make a new copy via cmp=plt.get_cmap('viridian') and modify the copy." ?

Unfortunately, that won't even work either! (which is why I think this is a bug) You would need to cmp=copy.copy(plt.get_cmap('viridian')) currently to get the desired functionality. The real problem is that further down if you go and request 'viridis' from a plot after doing a bunch of stuff, you will still get the bad colormap.

jklymak · 2021-05-11T15:53:03Z

Not sure if this is still the way we were planning to go here...

greglucas · 2021-05-11T16:06:32Z

I will close this as it was really meant for the 3.3/3.4 timeframe.

We should try to get #18503 (or a similar proposal) across the line sooner rather than later I think. Users (#19609) are confused about the deprecation warnings (partially my fault at not being clear in the messaging). So, getting over the hurdle of back-compat concerns would be beneficial here.

tacaswell added this to the v3.3.0 milestone Mar 28, 2020

anntzer added API: changes topic: color/color & colormaps labels Mar 28, 2020

tacaswell mentioned this pull request Mar 28, 2020

Modifying colormaps (as used in xarray) #16296

Closed

Changing get_cmap to return copies of the registered colormaps.

333bc06

greglucas force-pushed the get_cmap_copy branch from 102ce74 to 333bc06 Compare March 28, 2020 20:45

Adding a deprecation warning period for get_cmap().

56fc4ef

jklymak reviewed Mar 29, 2020

View reviewed changes

greglucas mentioned this pull request Apr 1, 2020

Begin warning on modifying global state of colormaps #16991

Merged

13 tasks

QuLogic mentioned this pull request May 4, 2020

ENH/API: improvements to register_cmap #15127

Merged

6 tasks

QuLogic modified the milestones: v3.3.0, v3.4.0 May 5, 2020

QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 22, 2021

jklymak marked this pull request as draft May 11, 2021 15:52

jklymak added the status: needs rebase label May 11, 2021

greglucas closed this May 11, 2021

greglucas mentioned this pull request May 14, 2021

Adding an equals method to colormaps #20227

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing get_cmap to return copies of the registered colormaps. #16943

Changing get_cmap to return copies of the registered colormaps. #16943

greglucas commented Mar 28, 2020

tacaswell commented Mar 28, 2020

anntzer commented Mar 28, 2020

tacaswell commented Mar 28, 2020

anntzer commented Mar 28, 2020

greglucas commented Mar 28, 2020

greglucas commented Mar 29, 2020

timhoffm commented Mar 29, 2020

timhoffm commented Mar 29, 2020

jklymak Mar 29, 2020

greglucas Mar 29, 2020

jklymak Mar 29, 2020

jklymak Mar 29, 2020 •

edited

Loading

greglucas Mar 29, 2020

jklymak commented May 11, 2021

greglucas commented May 11, 2021

Changing get_cmap to return copies of the registered colormaps. #16943

Changing get_cmap to return copies of the registered colormaps. #16943

Conversation

greglucas commented Mar 28, 2020

PR Summary

Implications

PR Checklist

tacaswell commented Mar 28, 2020

anntzer commented Mar 28, 2020

tacaswell commented Mar 28, 2020

anntzer commented Mar 28, 2020

greglucas commented Mar 28, 2020

greglucas commented Mar 29, 2020

timhoffm commented Mar 29, 2020

timhoffm commented Mar 29, 2020

jklymak Mar 29, 2020

Choose a reason for hiding this comment

greglucas Mar 29, 2020

Choose a reason for hiding this comment

jklymak Mar 29, 2020

Choose a reason for hiding this comment

jklymak Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

greglucas Mar 29, 2020

Choose a reason for hiding this comment

jklymak commented May 11, 2021

greglucas commented May 11, 2021

jklymak Mar 29, 2020 •

edited

Loading