Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Ensure name in set_cmap is contained in cmap_d#17012
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
@ianhi Thanks a lot for this. However wejust started looking at this.@greglucas is leading the charge. Can you co-ordinate with him? |
Sure. Sorry if I missed a relevant issue or PR before opening this. Can you point me in the right direction? |
#16991 is the most recent. |
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.
@ianhi, I think you're on the right track here in fixing this! Out of curiosity, did you run into the linked issue in your own workflow and use cmap_d for getting the Colormaps?
The larger background here is that getting and setting colormaps is not very safe right now because references are being passed around and lots of accidental overriding can take place. So, we are trying to eliminate the possibility of overwriting stored colormaps on accident. This means there will probably be quite a bit of refactoring and deprecations in these areas coming in the not-to-distant future.
In your current case, if I wanted to changeviridis to have different over/under values, I would have to register the Colormap under a different name.
cmap=copy.copy(plt.get_cmap('viridis'))cmap.set_under('k')plt.set_cmap(cmap)# Would actually still use the old 'viridis'
That wouldn't work currently because the cmap still has the name 'viridis' in it, which is already registered. We probably also need to add in a keyword argument to allow the user to overwrite the registered colormap if they want, but they have to explicitly request to do so.register_cmap(..., replace=False)
| elif isinstance(cmap, Colormap): | ||
| if cmap.name not in cm.cmap_d.keys(): | ||
| register_cmap(cmap.name, cmap) | ||
| rc('image', cmap=cmap.name) |
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.
Here you could get into another tricky situation.
Say someone hasalready registered a different Colormap under the same name. So, the cmap.name is in the dictionary already, but it is actually pointing to a different colormap and this wouldn't set the global cmap to what you're expecting either.
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.
Ah, this is tricky. Perhaps:
ifcmap.namenotincm.cmap_d:register_cmap(cmap.name,cmap)elifcmapisnotget_cmap(cmap.name):raisesomesortoferror/warning
and in that error/warning explain that they should useregister_cmap or change the name attribute of their colormap?
Also good to note that as originally written this function falls into the same trap. Though I suppose it's not super desirable to maintain backwards compatibility with a silent bug.
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.
Agreed, something like this would be desirable going forward to make it a little more clear what is going on and raising rather than silently doing something unexpected!
| cmap = get_cmap(name) | ||
| rc('image', cmap=name) | ||
| elif isinstance(cmap, Colormap): | ||
| if cmap.name not in cm.cmap_d.keys(): |
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.
You don't need the.keys() when checking if something is in a dictionary.
No, I was searching through issues for something else and when I saw this just couldn't let it go till I figured it out. Sorry if I've just added unnecessary noise. Though I will say I'm pretty sure I have at times been guilty of these unsafe colormap practices. Given the coming changes is it worth the effort to continue here? |
I would say that this is something thatshould be changed/updated. But, how to best approach the problem will (I think) depend on other decisions that are made in reference to how we are storing and getting the global colormaps. (i.e. I am guessing it will be a little while before a true review/merge would get done on this) Happy to have you help out and leave your feedback on any of the other ideas/issues open with colormaps if you want as well. |
I'm going to close in favour of#18503, but feel free to ask for a re-open if this is really different, and I've misunderstood. |
PR Summary
Fixes:#5087
Colormaps have their own name attribute, but this is not necessarily the name that they are registered with in
cm.cmap_d. This becomes a problem when usingset_cmfollowed byplt.imshowbecauseimshowexpects the name in the rcParams to also be present in the keys ofcmap_d. In the linked issue the poster noted that this can happen when using a colormap from a library.With this change
set_cmaptries to obtain the cmap fromcmap_donly if passed a string, and if passed a Colormap it will check if the cmap is registered, and if not register it usingcmap.name.I believe that this PR is backwards-compatible.
PR Checklist