Skip to content
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

[Bug]: mpl.colormaps[ "Grays" ].name is "Greys", not "Grays" #28114

Closed
denis-bz opened this issue Apr 21, 2024 · 7 comments · Fixed by #28116
Closed

[Bug]: mpl.colormaps[ "Grays" ].name is "Greys", not "Grays" #28114

denis-bz opened this issue Apr 21, 2024 · 7 comments · Fixed by #28116

Comments

@denis-bz
Copy link

Bug summary

A minor bug, with a simple fix: mpl.colormaps[ "Grays" ].name is "Greys", not "Greys"

Code for reproduction

print( f'{mpl.colormaps[ "Grays" ].name = }' )  # Greys

Actual outcome

mpl.colormaps[ "Grays" ].name = 'Greys'

Expected outcome

mpl.colormaps[ "Grays" ].name = 'Grays'

Additional information

A suggested fix: in cm.py, after lines 47 - 50

# Register colormap aliases for gray and grey.
cmap_d['grey'] = cmap_d['gray']
cmap_d['gist_grey'] = cmap_d['gist_gray']
cmap_d['gist_yerg'] = cmap_d['gist_yarg']
cmap_d['Grays'] = cmap_d['Greys']

change their .name s too:

cmap_d['grey'].name = 'grey'
cmap_d['gist_grey'].name = 'gist_grey'
cmap_d['gist_yerg'].name = 'gist_yerg'
cmap_d['Grays'].name = 'Grays'

Operating system

macos 10.15.7

Matplotlib Version

3.8.4

Matplotlib Backend

No response

Python version

3.10.0

Jupyter version

No response

Installation

pip

@timhoffm
Copy link
Member

It's a bit more involved than just changing the name attribute. There is currently only one underlying Colorbar instance for both. I see two possible resolutions:

  • Create explicit copies for the aliases. In this case we'd not really be aliasing anymore but just have two separate colormaps that happen to have the same data values.
  • Make the aliasing an explicit concept of ColormapRegistry.

Not sure which approach is better. The first is less complex, but we loose from-code accessible information on aliases (not that we have used that before). So slight tendency to go with the first. We can always build explicit aliases in later if we need to.

@tacaswell
Copy link
Member

My knee-jerk reaction was "can we just keep living with this", but looking into it a bit, if colormaps are added through ColorMapRegitry.register then we mutate the passed colormap to match the name we registered it as! I think the grays/grey miss-match exists because we pass the dictionary in rather than registering them one-by-one.

The __eq__ check on ColorMaps only looks at the values of the LUT (and if it is attached to a colarbar with extends which is odd but did not track down why) and ignores the name.

On __getitem__ we already return a copy so making a copy for the aliases is not a huge conceptual step and the users can't tell if we have one instance we are returning copies of two identical instances we are returning copies of.

This also suggests that rather than fixing the name on the way in, we should be fixing the name on the way out (as we have already paid to make a copy).

@tacaswell
Copy link
Member

This issue inspired me to do #28115

@timhoffm
Copy link
Member

My knee-jerk reaction was "can we just keep living with this"

That would be still an option.

On __getitem__ we already return a copy

This was added to prevent users from manipulating the builtin colormaps, e.g. via set_over(). There is the idea to make colormaps immutable eventually. While immuatbility is the better option, I'm unsure whether we want to enforce the associated API change. But if we do, the copy could go away.

This also suggests that rather than fixing the name on the way in, we should be fixing the name on the way out (as we have already paid to make a copy).

I don't follow that argument. The existing copy only means: We can fix the name of the way out without additional cost. I still think it's not the right way, because we have a sketchy internal state. self._cmaps['grey'].name == "gray" should not happen. Either we invest in the handful of additional copies on the way in (Option 1 from above). Or, we do not have _cmaps['grey'] and and let __getitem__ handle alias resolution. But this also means we have to specially handle aliases in keys, which brings us to the explicit alias concept (Option 2 from above).

timhoffm added a commit to timhoffm/matplotlib that referenced this issue Apr 22, 2024
@oscargus
Copy link
Contributor

I may be missing something, but isn't the idea of an alias that is should return the object that it aliases? Now, it is not so clear which is the original and alias here and maybe they are mixed-up? But the thing that an alias returns a different name than the alias name is expected?

@timhoffm
Copy link
Member

timhoffm commented Apr 22, 2024

I may be missing something, but isn't the idea of an alias that is should return the object that it aliases?

That depends on the interpertation. First and foremost, we want to give users the convenience that 'grey' and 'gray' are equally possible. AFAIK, we have not publically defined the mechanism how that is implemented. If we publically say "One is canonical and the other is just an alias for accessing plt.colormaps[name]", then the current behavior is ok. The alternative is to just have two identical colormaps with different names.

I think the second approach is more pragmatic, because plt.colormaps[name] != name is surprising and can only be understood if you know that we have aliases.

Note also that since we currently do copies on __getitem__, you never get the object. You'll always get new objects, which are equal but not identical.

@denis-bz
Copy link
Author

Thanks @timhoffm. Agree completely, immutability, readonly, is Good.

(What surprised me: I was plotting colormaps with ΔE color diffs, about like

for name, v in mpl.colormaps.items():
    if re.match( "Gr", name ):
        plot ... v.name ... )  # Greys twice ❓ 

@QuLogic QuLogic added this to the v3.10.0 milestone Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants