FIX: Allow different colormap name from registered name by greglucas · Pull Request #25479 · matplotlib/matplotlib · GitHub

PR Summary

When registering a colormap you can use a different name than the cmap.name attribute now. This will set the colormap based on the registered name rather than cmap.name.

closes #5087

PR Checklist

Documentation and Tests

Has pytest style unit tests (and pytest passes)
[-] Documentation is sphinx and numpydoc compliant (the docs should build without error).
[-] New plotting related features are documented with examples.

Release Notes

[-] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
[-] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
[-] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

Can this be clearer which "this" is meant here? I almost commented based on this comment that the next line should be cmap = get_cmap(name), but am no longer sure which was intended.

I think we want to use get_cmap(cmap) here in case someone didn't register the colormap? Although that would also mean the rc paramter wouldn't work then either...

Perhaps clearer to return a nonesense variable here? Even with he comments, I thought the next line was a typo...

Suggested change

      
                cmap = get_cmap(cmap)
          
                _ = get_cmap(cmap)

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.name
plt.set_cmap(my_cmap)  # note, passing instance, not name

Won't this get "viridis" from the getattr, and thus set that as the rc cmap? (It will apply the "correct" cmap to gci, 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't super 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:

I think we want to use get_cmap(cmap) here in case someone didn't register the colormap? Although that would also mean the rc paramter wouldn't work then either...

Question: Is it OK to assign a colormap object to the image.cmap rcParam? I think that is the only way we can do this because otherwise, we get what Kyle was mentioning above with the mismatched registered vs actual cmap name.

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.

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 from bb2de5e 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?

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.

I'm 👎 on registering a colormap with a name different from its original name. This is confusing up to dangerous.

By default colormaps.register(cmap) uses cmap.name. The only reason to use register(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 that ColormapRegistry.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 to rcParams. 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 set image.cmap to a Colormap 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 cmap1.name == cmap2.name in the __eq__ method, which really shouldn't matter from a comparison perspective, we should only bother with the lookup tables which are what visually identify the colormaps being equal.

Modulo, restoring the equal test.

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 than the "cmap.name" attribute now. This will set the colormap based on the registered name rather than cmap.name, updating the copy of the object that gets registered. Additionally, the equals method of Colormaps shouldn't care about the name of the object, we should only worry about whether the values are the same in the lookup tables and let someone use object identity if they are worried about the name of the object.

greglucas added topic: color/color & colormaps PR: bugfix Pull requests that fix identified bugs labels Mar 16, 2023

greglucas added this to the v3.7.2 milestone Mar 16, 2023

tacaswell reviewed Mar 16, 2023

View reviewed changes

greglucas force-pushed the cmap-bad-name branch from 4fbc57d to 16a8477 Compare March 16, 2023 18:33

tacaswell approved these changes Mar 16, 2023

View reviewed changes

jklymak reviewed Mar 16, 2023

View reviewed changes

ksunden reviewed Mar 16, 2023

View reviewed changes

greglucas commented Mar 16, 2023

View reviewed changes

greglucas modified the milestones: v3.7.2, v3.8.0 Mar 18, 2023

timhoffm requested changes Apr 23, 2023

View reviewed changes

greglucas force-pushed the cmap-bad-name branch from 40a321f to e327961 Compare April 25, 2023 02:25

timhoffm approved these changes Apr 25, 2023

View reviewed changes

greglucas force-pushed the cmap-bad-name branch from e327961 to cc977dc Compare April 25, 2023 14:17

ksunden approved these changes Apr 25, 2023

View reviewed changes

ksunden merged commit 63c97a2 into matplotlib:main Apr 25, 2023

greglucas deleted the cmap-bad-name branch April 25, 2023 20:30

dstansby mentioned this pull request Sep 16, 2023

Fix colormap name in map tutorial sunpy/sunpy#7197

Merged

pauldmccarthy mentioned this pull request Sep 22, 2023

KeyError: 'fsleyes_pink' pauldmccarthy/fsleyes#120

Closed

+                  # 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

+                  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)

-                  # Test different names are not equal
-                  cm_copy = cmap.copy()
-                  cm_copy.name = "Test"
-                  assert cm_copy != cmap

                   cmap = get_cmap(cmap)
-                  rc('image', cmap=cmap.name)
+                  rc('image', cmap=name)

FIX: Allow different colormap name from registered name by greglucas · Pull Request #25479 · matplotlib/matplotlib · GitHub

Repository navigation

FIX: Allow different colormap name from registered name - #25479

FIX: Allow different colormap name from registered name#25479ksunden merged 1 commit intomatplotlib:mainmatplotlib/matplotlib:mainfrom greglucas:cmap-bad-namegreglucas/matplotlib:cmap-bad-nameCopy head branch name to clipboard

Conversation

greglucas commented Mar 16, 2023

PR Summary

PR Checklist

Uh oh!

tacaswell Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

greglucas Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

jklymak Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

ksunden Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

greglucas commented Mar 16, 2023

Uh oh!

greglucas Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Mar 16, 2023

Uh oh!

greglucas commented Mar 16, 2023

Uh oh!

greglucas commented Mar 18, 2023

Uh oh!

timhoffm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greglucas commented Apr 25, 2023

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

timhoffm Apr 25, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FIX: Allow different colormap name from registered name#25479
ksunden merged 1 commit into
matplotlib:mainfrom
greglucas:cmap-bad-name

timhoffm left a comment •
edited
Loading