Repository navigation
Ensure name in set_cmap is contained in cmap_d - #17012
ianhi wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
@ianhi Thanks a lot for this. However we just started looking at this. @greglucas is leading the charge. Can you co-ordinate with him? |
Sorry, something went wrong.
|
Sure. Sorry if I missed a relevant issue or PR before opening this. Can you point me in the right direction? |
Sorry, something went wrong.
greglucas
left a comment
There was a problem hiding this comment.
@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 change viridis 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)
Sorry, something went wrong.
| 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.
Here you could get into another tricky situation.
Say someone has already 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.
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, this is tricky. Perhaps:
if cmap.name not in cm.cmap_d:
register_cmap(cmap.name, cmap)
elif cmap is not get_cmap(cmap.name):
raise some sort of error/warningand in that error/warning explain that they should use register_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.
Sorry, something went wrong.
There was a problem hiding this comment.
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!
Sorry, something went wrong.
| 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.
You don't need the .keys() when checking if something is in a dictionary.
Sorry, something went wrong.
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? |
Sorry, something went wrong.
|
I would say that this is something that should 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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 using set_cm followed by plt.imshow because imshow expects the name in the rcParams to also be present in the keys of cmap_d. In the linked issue the poster noted that this can happen when using a colormap from a library.
With this change set_cmap tries to obtain the cmap from cmap_d only if passed a string, and if passed a Colormap it will check if the cmap is registered, and if not register it using cmap.name.
I believe that this PR is backwards-compatible.
PR Checklist