Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Changing get_cmap to return copies of the registered colormaps.#16943
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I less disruptive approach to this would be to add a 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 |
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. |
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. |
I think we are all on the same page that this is whatshould 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 My thought is that this is abug 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') |
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? |
@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. |
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. |
| 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 |
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.
"... copy of the requested colormap..."?
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.
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.
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.
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.
| "colormap in this way has been deprecated. " | ||
| "Please register the colormap using " | ||
| "plt.register_cmap() before requesting " | ||
| "the modified colormap.") |
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.
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." ?
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.
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.
Not sure if this is still the way we were planning to go here... |
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. |
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.Previously, a second call to get the viridis colormap would return the modified colormap.
cmap == plt.get_cmap('viridis') # TrueNow, it will return the original viridis built-in colormap.
cmap == plt.get_cmap('viridis') # FalseImplications
This also means that there are two copies of the colormap objects created initially now (one in
cmap_dand a second deepcopy that I made forlocals()to use with the pyplot interface.Is there a way to make the
plt.cm.viridisobject point to the functionplt.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