Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
FIX: Allow different colormap name from registered name#25479
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
lib/matplotlib/pyplot.py Outdated
| # The name of the colormap may be different than the registered name. | ||
| # We want the registered name for the rc setting | ||
| name = getattr(cmap, "name", cmap) | ||
| # Run this through get_cmap to confirm this is a valid cmap before |
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.
Can this be clearer which "this" is meant here? I almost commented based on this comment that the next line should becmap = get_cmap(name), but am no longer sure which was intended.
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.
Ithink we want to useget_cmap(cmap) here in case someone didn't register the colormap? Although that would also mean the rc paramter wouldn't work then either...
lib/matplotlib/pyplot.py Outdated
| # We want the registered name for the rc setting | ||
| name = getattr(cmap, "name", cmap) | ||
| # Run the cmap (str or Colormap) through get_cmap to validate | ||
| # the cmap before updating the rc parameters | ||
| cmap = get_cmap(cmap) |
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.
Perhaps clearer to return a nonesense variable here? Even with he comments, I thought the next line was a typo...
| cmap=get_cmap(cmap) | |
| _=get_cmap(cmap) |
lib/matplotlib/pyplot.py Outdated
| cmap = get_cmap(cmap) | ||
| rc('image', cmap=cmap.name) | ||
| rc('image', 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.
If I do:
my_cmap=mpl.colormaps["viridis"].copy()my_cmap.set_bad("FF00FF")mpl.colormaps.register("my-cmap",my_cmap)# note, did not update cmap.nameplt.set_cmap(my_cmap)# note, passing instance, not name
Won't this get"viridis" from thegetattr, and thus set that as the rc cmap? (It will apply the "correct" cmap togci, but not to future mappables)
Not sure how to square that without a) doing a reverse lookup in the registry or b) requiring that the names match when registered.
Perhaps this change is still better than no change, but not sure it's without its rough edges still... certainly it makes it possible to use a mismatched cmap, but you do need to get it by the registry name, not passing the cmap object.
Of course, a bug that has been hanging around for a while isn'tsuper easy to fix. @ksunden, good catch and explanation about that edge-case. You said it much better than me in one of the other comments:
Question: Is it OK to assign a colormap object to the I just pushed up a commit with that, which I think is the more robust way to do this, but all other rcParams I think are strings, so I'm not sure if this is OK during runtime. |
lib/matplotlib/pyplot.py Outdated
| cmap = get_cmap(cmap) | ||
| rc('image', cmap=cmap.name) | ||
| rc('image', cmap=cmap) |
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.
See here for changing the string name to the object itself.
I was about to say I was wary of just putting the colormap object in rcparams....but the validator allows it frombb2de5e so we support that from 3.4 on. Maybe the logic should be if the named lookup is itself then use the string otherwise the object? |
I think we'd need to add a bit of unsightly code and try/except for this case because the name of the cmap may not be in the lookup table if it was registered under a different name, thus throwing a ValueError cmap is not a valid name. It seems like quite a bit of complication to add and I'm not sure how much the string nature matters here since we are only doing this during runtime? Unless someone is exporting the rcParams to a file and hoping to reimport them later with a specific string... |
Since I changed this to updating the rcParams with an object instead of string, I've remilestoned to 3.8 instead because some people may be relying on rcParams of a certain type and it doesn't seem quite right for a patch release and this bug has been around for a while already. Happy to change back if people think otherwise. |
timhoffm left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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'm 👎 on registering a colormap with a name different from its original name. This is confusing up to dangerous.
By defaultcolormaps.register(cmap) usescmap.name. The only reason to useregister(cmap, name=other_name) is that the user intentionally wants to register the colormap under a different name. We can interpret this as authorization to rename the colormap itself as well.
Note thatColormapRegistry.register() stores a copy of the given colormap to avoid any later modifcations. Since renaming would only apply to this copy, it cannot have unintended side-effects.
I regard this as a bug-fix. It can be applied immediately.
I'm somewhat sceptical assigning complex objects torcParams. Even though this is technically supported through the validator, I would not go down this road further without need (and I think with the above renaming, there is no need to actively setimage.cmap to aColormap from our side).
@timhoffm, thank you for taking a look at this and coming up with that suggestion! I like the idea of setting the name on the object that gets registered to keep it consistent and I think that flows nicely with expectations from an end-user perspective (I forgot we make a copy upon registering now). I've updated the code accordingly. Note that I also removed |
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.
Modulo, restoring the equal test.
lib/matplotlib/tests/test_colors.py Outdated
| # Test different names are not equal | ||
| cm_copy = cmap.copy() | ||
| cm_copy.name = "Test" | ||
| assert cm_copy != cmap |
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 suggest to keep this and test that colormaps with different names are still equal.
When registering a colormap you can use a different name thanthe "cmap.name" attribute now. This will set the colormap basedon the registered name rather than cmap.name, updating the copyof the object that gets registered.Additionally, the equals method of Colormaps shouldn't care aboutthe name of the object, we should only worry about whether thevalues are the same in the lookup tables and let someone useobject identity if they are worried about the name of the object.
PR Summary
When registering a colormap you can use a different name than the
cmap.nameattribute now. This will set the colormap based on the registered name rather thancmap.name.closes#5087
PR Checklist
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst