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

[ENH]: Default matplotlib.colormaps[None] to call matplotlib.colormaps[matplotlib.rcParams['image.cmap']]? #23981

Closed
mroeschke opened this issue Sep 21, 2022 · 6 comments · Fixed by #24111

Comments

@mroeschke
Copy link

Problem

While addressing the matplotlib.cm.get_cmap deprecation in 3.6:

PendingDeprecationWarning: The get_cmap function will be deprecated in a future version. Use ``matplotlib.colormaps[name]`` instead.

I noticed that None isn't directly migrate-able

In [1]: import matplotlib

In [2]: matplotlib.cm.get_cmap(None)
Out[2]: <matplotlib.colors.ListedColormap at 0x11e609e20>

In [3]: matplotlib.colormaps[None]
KeyError: 'None is not a known colormap name'

Proposed solution

It appears from the source that get_cmap(None) defaults to matplotlib.rcParams['image.cmap'] so it would be nice if colormaps[None] could default to that as well.

Otherwise, it would be nice if this was better documented.

@story645
Copy link
Member

story645 commented Sep 21, 2022

If folks agree it's a regression error (& I think it kinda is) the fix is I think add something like

 if item is None: 
     item = mpl.rcParams['image.cmap'] 

which is a direct port of the get_cmap code

if name is None:
name = mpl.rcParams['image.cmap']

before the try in:

def __getitem__(self, item):
try:
return self._cmaps[item].copy()
except KeyError:
raise KeyError(f"{item!r} is not a known colormap name") from None

@timhoffm
Copy link
Member

timhoffm commented Oct 2, 2022

@mroeschke What is your use case? Are you passing in None as a literal or is it the value of some variable you have? Is this in library code or do you expect this to be needed in an interactive console.

We have discussed this on the last dev call and are not yet clear how to handle the default colormap case in the new API. Logically, colormaps itself was designed as a mapping of all explicit colormaps. The default colormap notion does not fit in there very well. It depends on the anticipated usage how we would handle that API-wise.

@mroeschke
Copy link
Author

The use case would be for library code. In pandas, there are some APIs with matplotlib functionality that accept cmap that default to None and were passed into matplotlib.cm.get_cmap: https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.io.formats.style.Styler.bar.html

We're happy to start using matplotlib.colormaps instead just noting in #23981 (comment) that I had to dig into what get_cmap(None) did before to maintain the prior behavior.

@tacaswell
Copy link
Member

@mroeschke I was going to suggest that you do something like

cmap = cmap in cmap is not None else mpl.rcparams['image.cmap']

which should work on all versions of Matplotlib and leaves you an escape hatch if you want to make the default colormap controllable indepently. However, that probably is not enough for you as if you take str or ColorMap and actually want the color map (rather than passing through to some mpl function) you'll also have to have the logic to decide if you need to look up the color map or if the user directly gave you one.

When we decided to move to the registry model we mostly had end-users in mind, however this issues highlights that we did not take into account the library case enough. We have

def _ensure_cmap(cmap):
"""
Ensure that we have a `.Colormap` object.
Parameters
----------
cmap : None, str, Colormap
- if a `Colormap`, return it
- if a string, look it up in mpl.colormaps
- if None, look up the default color map in mpl.colormaps
Returns
-------
Colormap
"""
if isinstance(cmap, colors.Colormap):
return cmap
cmap_name = cmap if cmap is not None else mpl.rcParams["image.cmap"]
# use check_in_list to ensure type stability of the exception raised by
# the internal usage of this (ValueError vs KeyError)
_api.check_in_list(sorted(_colormaps), cmap=cmap_name)
return mpl.colormaps[cmap_name]
for internal use (which is a bit more complicated than it needs to be to preserve exact error types). It is not clear to me if we should make that public or document the pattern of:

if isinstance(inp, mcolors.Colormap):
    cmap = inp
else:
    cmap = mpl.colormaps[inp if inp is not None else mpl.rcParams["image.cmap"]]

There is just enough complexity in there it probably be a function we provide. I think this has to be a function/method rather than shoehorned into __getitem__ or get on the registry because that would be getting too far away from the Mapping API

If we have a registry method for this I propose

def ensure_colormap(self, cmap: str|Colormap, * default=None):
    if isinstance(cmap, mcolors.Colormap):
       return cmap

    default = default if default is not None else mpl.rcParams["image.cmap"]
    cmap = cmap if cmap is not None else default
    return self[cmap]

which could also live as a free method and use the global registry.

@mroeschke
Copy link
Author

Yeah in pandas we used a similar workaround for translating cm.cmap(None) to remove the deprecation warning.

I don't have any strong opinions personally on colormaps[None] being valid. I mainly wanted to highlighting that the deprecation message wasn't entirely clear for the name=None case.

@tacaswell
Copy link
Member

Discussed on the call, we are going to put the method on the registery class for 3.6.1 on the reasoning that this is not a new feature, but completing something that should have been in 3.6.1 as part of the deplication.

@QuLogic QuLogic added this to the v3.6.1 milestone Oct 6, 2022
tacaswell added a commit to tacaswell/matplotlib that referenced this issue Oct 6, 2022
After putting pending deprecations on `cm.get_cmap` we discovered that
downstream libraries (pandas) were using the deprecated method to normalize
between `None` (to get the default colormap), strings, and Colormap instances.
This adds a method to `ColormapRegistry` to do this normalization.

This can not replace our internal helper due to variations in what exceptions
are raised.

Closes matplotlib#23981
tacaswell added a commit to tacaswell/matplotlib that referenced this issue Oct 6, 2022
After putting pending deprecations on `cm.get_cmap` we discovered that
downstream libraries (pandas) were using the deprecated method to normalize
between `None` (to get the default colormap), strings, and Colormap instances.
This adds a method to `ColormapRegistry` to do this normalization.

This can not replace our internal helper due to variations in what exceptions
are raised.

Closes matplotlib#23981
tacaswell added a commit to tacaswell/matplotlib that referenced this issue Oct 7, 2022
After putting pending deprecations on `cm.get_cmap` we discovered that
downstream libraries (pandas) were using the deprecated method to normalize
between `None` (to get the default colormap), strings, and Colormap instances.
This adds a method to `ColormapRegistry` to do this normalization.

This can not replace our internal helper due to variations in what exceptions
are raised.

Closes matplotlib#23981
melissawm pushed a commit to melissawm/matplotlib that referenced this issue Dec 19, 2022
After putting pending deprecations on `cm.get_cmap` we discovered that
downstream libraries (pandas) were using the deprecated method to normalize
between `None` (to get the default colormap), strings, and Colormap instances.
This adds a method to `ColormapRegistry` to do this normalization.

This can not replace our internal helper due to variations in what exceptions
are raised.

Closes matplotlib#23981
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.

5 participants