-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
DeprecationWarning when changing color maps #19609
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
Comments
Didn't 3.3 also have this behaviour? This came in on #16991 with lots of discussion of this exact point. I actually do not feel that discussion was adequately resolved, so I guess we should rehash it. In my opinion, I disagree with the idea of making the global unmutable. I think if you modify a global, you meant to modify it, and that we should provide a method to return a copy. The original idea of #16991 was that if you change the "over" color of the global |
I have been dealing with a number of additional warnings that I only noticed when updating my package to use v3.4.0c1. It is possible this also occurred in v3.3 and I apologize for not spending more time understanding the history of this change before posting. It seems as if part of the debate concerns internal plumbing that I am not familiar with. I got the impression that a call to
It seems to me that, whatever the underlying design, this should just work. |
Well, that is another wrinkle, I guess. Lets suppose I am good and make a copy of cmap before hand... import matplotlib.cm as cm
import copy
local_cmap = copy.copy(cm.RdBu_r)
im = ax.pcolormesh(x, y, data, cmap=local_cmap)
local_cmap.set_bad('k', 'organge') Do I expect the set_bad to change the artist? I agree that |
I think that shows why the I am guessing that the origin of this controversy is that, if in the past people have assumed that your code will change |
@rayosborn unless I misunderstood something, I think what we are trying to do is in line with your general ideas. We have the design flaw that by default all images / colormaps you use are references to the builtin colormap objects. Thus any changes like To prevent users from modifying the global colormaps, there are two ways a) Make the colormaps immutable. While this could be conceptually attractive, it's a larger API change, that would need strong justification. We're not following this path for now. We're following path b) as laid out in #16991. Unfortunately, there are vailid use-cases that may have relied on modifcation of the global state, e.g.
To not break user code without prior notification, we needed the warning you've been seing. Your example #19609 (comment) will continue to work. We only cannot distinguish the cases and therefore are erring on the side of warning too much than too little. The change to copy should have been implemented for 3.4, but we missed that and the RC is out, so we will likely extend the period one release and change to copy on 3.5. The warning will stay in-place for so long. The copy variant is developed in #18503. Apparently, that has been re-milestoned to 3.5 anyway. |
I may have misunderstood @jklymak's comment, but it seemed to suggest that there is still a debate about the issue. So with v3.5, can I assume that any changes to
However, I'm not sure I entirely agree though with the decision to issue these warnings. If I understand it, this design flaw has existed for years. To remove the warning message, I will have to modify my own package to make a copy of the color map for v3.4, but then remove this code when v3.5 is released. Since I want my package to be backwardly compatible, I will have to do a version check to modify the package behavior for the sake of a single Matplotlib version. I suppose this only affects one or two package maintainers, so if you think the warning is required for interactive users, I can understand why you do it. |
Yes.
The warning is already in 3.3. More importantly, your code is currently modifying the global 'viridis' colormap. If somebody uses viridis after your code, he will have the modified cmap. This is rather Matplotlibs fault than yours and as discussed here we'll fix that on our side, but that will take till 3.5 to not break other legitimate usages without prior notice. Not primarily to suppress the warning, but in the interest of your users, you should make a copy. Note also that we'll be making the simple Your code should look like this.
This is will work for all existing and future versions of Matplotlib. I wouldn't even bother to do a version check for 3.5. The only disadvantage is that you do an unnecessary extra copy, but that's cheap enough. Once you will only support matplotlib 3.5+ you'll probably be able to simplify this to something like
But we're still working on the details.
This also affects interactive users. Consider somebody using your code example or
in a jupyter notebook. This modifies the global 'viridis' cmap for the reset or the notebook and may very well not be intended. |
Have we considered adding a kwarg to |
AFAIK a copy kwarg has not been discussed. However, I don't think it's the right thing here:
|
@timhoffm, thanks for the detailed clarification. I will rewrite the NeXpy code to create copies where necessary as you suggest, at least until v3.5 is old enough to become a dependency. Could I ask one quick question since we are on the subject? If |
@timhoffm Asking folks to
It would be nice to see the full proposal for how that will work and what the final API will be. I don't think the desirability of removing global state is particularly up for debate, but how the colormap scope is handled below global is still not clear to me.
I think the idea is to provide a Registry class that will presumably allow a view of the keys... #18503 |
See #19663 for what I think is a better temporary state of affairs. |
We have not yet decided on the final API, but the basic plan is laid out at the top of #16991, with the exception that we have semi-intentionally pushed the changes mentioned for 3.4 to 3.5. Essentially, we'll never give access to the global colormap directly but will always return a copy. Then, the user can do what ever she wants with that. Whether or not we make colormaps immutable on top of that later is basically an independent discussion. Conceptually, immutablility would be nice - you can anyway only modify over/under/bad, the regular [0, 1]->color mapping cannot be changed via public API. However, I feel that deprecating set_over/under/bad would break too much user code. Without having made up my mind completely, I guess we could encourage people to treat colormaps as immutable and only use |
I find #16991 a bit too ambiguous. What is meant by "still return global"? From So far it seems that we will allow colormaps to remain constant in user scope and below, so that regardless of where
will pass, and modifications to cmap will affect both im1 and im2. This is currently the case, and I guess I agree we shouldn't change it. The plan I believe, is to make
will fail. That seems right to me. However, I wonder if a lighter touch would be to keep I know that is not quite as protective as just not allowing the global to be modified, but it accomplishes a few things - it provides an easy way to make copies, which is what we want the user to do, and it makes it possible to get a safe version of the colormap. So:
|
I'm not sure about that. I've considered But the case of
seems reasonable. We could make
is worst of all. I could imagine that the least offensive way forward is to have an internal flag
|
If we make a copy of the cmap on set or init then we make it impossible to intentionally couple artists where as if we keep a reference it is possible to have coupled or uncoupled at the users choice. |
I think this is all covered by the plan in #20853, so no need to have a second issue tracking that. |
Summary
In v3.4.0rc1, calls to a colormap's
set_bad
,set_under
, andset_over
functions now trigger a deprecation warning saying that modifying a registered color map will soon be disallowed. This seems overly protective. If users have called any of these routines, then they are presumably doing so intentionally, probably after reading the documentation. This seems like the kind of routine customization that any user might expect to perform without going through the hoop of copying a color map every time.Proposed fix
Remove the calls to
_warn_if_global_cmap_modified(self)
in theColormap.set_bad
,set_under
, andset_over
functions incolor.py
and change future plans to disallow such calls.The text was updated successfully, but these errors were encountered: