Skip to content

Add a dedicated ColormapRegistry class #18503

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

Merged
merged 2 commits into from
Aug 15, 2021

Conversation

timhoffm
Copy link
Member

PR Summary

Following the thoughts in #16991 (comment), this defines a Mapping subclass ColorbarRegistry with the following properties:

  • read-only mapping: name -> Colormap. - Adding new colormaps is for now still done via register_cmap(). The class will grow the ability to add elements later, but that needs more API consideration.
  • The returned Colormaps are copies, so that the global state is not affected by modification to the returned colormaps. - This is the goal for get_cmap() anyway (see its deprecation note).

This is a minimal version, that allows to get the registered colormap names. Closes #18490.

Still to be decided: Where should the instance colormaps instance live? In cm, in matplotlib? Should cm be integrated into colors anyway?

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@story645
Copy link
Member

I vote leave it in cm for now, but since there's HTML colorbar reprs now (#17888) can you use this to print the list of colormaps?

@timhoffm
Copy link
Member Author

You mean as ColormapRegistry.__repr_html__? In principle yes. - I'm always a bit cautious on long reprs.

@story645
Copy link
Member

I hear but like I have that page of the docs printed out cause of how often I look up the colormaps.

@jklymak
Copy link
Member

jklymak commented Sep 17, 2020

guess I'm not particularly following the use-case for this versus just get_cmap? If we just want a list of colormaps, why can't we just store that as a flat list and give the user access to it? I'm not following why we need a mapping.

@timhoffm
Copy link
Member Author

This is motivated in the discussion #16991 (review).

The idea is to have a more concise colormap mangement and API. The current state is a bit ad-hoc. You have a private (formerly public) dict and functions get_cmap, register_cmap working on that. By making the dict private, users lost access to the keys.

You could store the names in a flat list, but that would be redundant to the keys of the private dict and needs synchronization. Also, users could manipulate that list - though I'm not too concerned about this.

The whole construct is conceptually a mapping. So why not make it one? Collecting everything into one class makes it easier to manage everything consistently. It makes it also easier to move the user-facing actions out of cm. While to be thought about in more detail, I can imagine that the colormaps can be accessed in the future from the top-level module such as:

import matplotlib as mpl
mpl.colormaps.register(my_colormap)
cmap = mpl.colormaps['jet']

@jklymak
Copy link
Member

jklymak commented Sep 17, 2020

OK, so this is just a first step to this class being a new interface for the colormap registry. Because right now, if I use cmap_register this class will no longer return the new colormap, right? I guess I think at the very least it should return the current state of the registry, not the state at import.

I'd prefer not having to import cm to get this list. I always import pyplot, so I'd be fine with it there, but I understand the aesthetic problem with "we discourage pyplot, except for these 4 things we still think are useful".

@timhoffm
Copy link
Member Author

timhoffm commented Sep 17, 2020

OK, so this is just a first step to this class being a new interface for the colormap registry.

Correct.

Because right now, if I use cmap_register this class will no longer return the new colormap, right? I guess I think at the very least it should return the current state of the registry, not the state at import.

It does. The internal self._cmaps is a reference to the same object as _cmap_registry:

In [9]: cm.colormaps._cmaps is cm._cmap_registry
Out[9]: True

Thus, the object plays nicely with the existing functions. (Whether or not we deprecate and remove them or just discourage their use is still to be decided.)

I'd prefer not having to import cm to get this list. I always import pyplot, so I'd be fine with it there, but I understand the aesthetic problem with "we discourage pyplot, except for these 4 things we still think are useful".

Agreed. There should be no need for the user to import cm. The nice thing with an object is that we can easily reference it from anywhere. We can instantiate it in cm but do from matplotlib.cm import colormaps in matplotlib and pyplot (or we directly instantiate it in matplotlib and only import in pyplot.

@greglucas
Copy link
Contributor

👍 I like this. I think it makes it clear what is going on and allows for flexibility moving forward.

What would you think about adding a register() method directly to this class? It seems like people would want to add/write directly to this registry.
mpl.colormaps.register(my_new_cmap)
mpl.cm.register_cmap() could just dispatch directly to that method then.

Or even allow people to set the colormaps just like a dict, but raise on name collisions?
mpl.colormaps['viridis'] = my_new_cmap -> raise

@timhoffm
Copy link
Member Author

timhoffm commented Sep 20, 2020

@greglucas That's exactly the plan. I'm not completely decided on register() vs. dict-setitem, which is why it's not yet in the PR. I tend towards an explicit register() because we likely won't have a full mutable mapping functionality (or semantic, e.g. concerning overwriting existing keys).

@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 21, 2020
@jklymak jklymak removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 5, 2021
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021
@jklymak jklymak mentioned this pull request Mar 7, 2021
7 tasks
@greglucas
Copy link
Contributor

@timhoffm, is there anything here you want help with or feedback on? I'd like to see this get in for the 3.5 release if possible.

@timhoffm timhoffm force-pushed the colormap-registry branch 5 times, most recently from a19b3ba to 7f63ce9 Compare May 14, 2021 01:20
@timhoffm timhoffm force-pushed the colormap-registry branch from ce7f656 to 79f414a Compare August 1, 2021 22:51
@timhoffm timhoffm marked this pull request as ready for review August 1, 2021 22:55
@timhoffm
Copy link
Member Author

timhoffm commented Aug 1, 2021

Note: I've added register(..., force=False).

It is currently not possible to override builtin colormaps via the ColormapRegistry API. That's intentional. I'm questioning the need for such an operation. For now, people can still use cm.register_cmap(..., override_builtin=True) if really needed (or overwrite ColormapRegistry._cmaps).

To make mpl.colormaps work, I had to do a late import of cm because that depends on rcParams to be set up. I intend to clean this all up later. But that requires reorganization of the rcParams code, which would be too much for 3.5. Nevertheless, I'd like to get the colormaps API into 3.5. Hence the workaround.

@timhoffm timhoffm force-pushed the colormap-registry branch from 79f414a to 6049f54 Compare August 1, 2021 23:30
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall 👍 I think it is good to have this easily available at the top-level namespace.

It took me a while to find this information in the docs pages. I think it could be added to the main Colormaps docs section. Right now, those all still use mpl.cm.get_cmap().
https://62710-1385122-gh.circle-artifacts.com/0/doc/build/html/tutorials/colors/colormaps.html#sphx-glr-tutorials-colors-colormaps-py

Another area that is missing is how to register a cmap after creating it. The "Creating colormaps in matplotlib" section just shows how to create a colormap, perhaps a final section describing "this is how to register a colormap for future use"
https://62710-1385122-gh.circle-artifacts.com/0/doc/build/html/tutorials/colors/colormap-manipulation.html#creating-colormaps-in-matplotlib

@timhoffm timhoffm force-pushed the colormap-registry branch from 6049f54 to 40f4013 Compare August 5, 2021 20:31
@timhoffm
Copy link
Member Author

timhoffm commented Aug 5, 2021

It took me a while to find this information in the docs pages.

Docs certainly need adaption. However, I first want to make sure that the API is agreed on before starting to change the docs.
Also technically, we could have this in 3.5 without further doc changes. It's properly announce and the old way is not deprecated (only discouraged). It would also be ok to change the docs in 3.5.1 if we don't manage that for 3.5.

@jklymak
Copy link
Member

jklymak commented Aug 10, 2021

This seems fine to me. I feel that we will still have some confusion, as the colormap registry is ephemeral, so why have so much public interface to it. If I need to create the colormap at startup, I'd just use it directly. So I think the added docs will be helpful to guide folks and make it clear what is happening here.

mpl.colormaps.register(my_colormap)
"""
def __init__(self, cmaps):
self._cmaps = cmaps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense why to do this as to wrap this around the existing dict, but I am wary of sharing the underlying storage this way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very intentional, that the registry shares the underlying storage: For a transition period, the registry and the cmap functions will coexists. Due to API quirks in the functions, it's better if both can directly work on the underlying storage and not one API calling the orther.

If you only have concerns with the constructor accepting a prebuilt mapping, we can change it to creating an empty registry by default and have a private ColormapRegistry._from_dict() that we use to inject the existing dict.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be happy warning in the init docstring saying we keep a reference to the input dict.

It makes total sense why you are doing it this way, just set off alarm bells in my head (we have had some cases at BNL where we did not mean to do this and ended up with hard-to-debug action-at-a-distance bugs!).

return ('ColormapRegistry; available colormaps:\n' +
', '.join(f"'{name}'" for name in self))

def register(self, cmap, *, name=None, force=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need an deregister as well? with force you can over-write an existing colormap, but I still think we will want a way to remove a key.

I do not think we want to promote this to a MutableMapping and support the full set of del / pop / clear. The act of adding and removing color maps is a different activity than the act of getting a colormap out of the registry.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that some sort of deregister should be there at some point. Details would need to be discussed, e.g. if you can deregister built-in cmaps (I think that should not be possible these are reseverd names with fixed meaning and no public API should change them).

Anyway, we currently don't have a deregister function, so there is no hurry with adding that new feature in the initial version of the registry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have unregister_cmap which went out in 3.4 via #15127

def unregister_cmap(name):
"""
Remove a colormap recognized by :func:`get_cmap`.

@tacaswell
Copy link
Member

My sense is that long term we want to deprecate mpl.cm.register_colormap and friends? There will have to be some period in the middle where we support both so if that is the plan I'm not worried about the confusion factor.

Getting a bit over my skis, having these in a nice object at least opens a path putting filter(...) methods on it that will return a new one with just the color maps of interest (e.g. ask only for perceptually uniform or diverging color maps...assuming we tag them with enough information) which would be super useful for a UI.

@tacaswell
Copy link
Member

I do not think we are going to get this done in the next few weeks, I propose bumping it to 3.6.

@greglucas
Copy link
Contributor

I forget where we stand on deprecating the global colormap mutability and whether this PR is something that needs to get in for that?

This PR mentions that we missed the intermediate state and extended the deprecation period: #19766, but I'm not clear if we ever added the intermediate state in 3.5 or not? i.e. is there an action item to add/change the way we get colormaps/warn about it?

xref: #19609 for a discussion about mpl.cm.{cmap_name}.

@timhoffm
Copy link
Member Author

timhoffm commented Aug 14, 2021

Overall goal

The registry is a more natural structure for managing colormaps. We have data and corresponding functions that are tightly related. IMHO it's a much nicer API for the user and easier to extend. And we get rid of some API quirks in the functions like get_cmap(.. lut=...) or the parameter order register_cmap(name, cmap) where name is optional.

Target state

  • the matplotlib.cm colormap functions should eventually be removed.
  • I'm open for discussion if we keep the pyplot functions or replace them with the registry as well.

Transition path

While I would like to eventually remove the matplotlib.cm functions, I propose a prolonged transition path because they are quite fundamental functions also used in third party libraries:

  • For 3.5: introduce the registry as is in this PR; discourage the cm functions. This is to see if we have missed something important in the design and get user feedback. - Maybe we should declare the registry experimental.
  • For 3.6: Introduce a pending deprecation. This is the time where all downstream libraries should start switching to using the registry.
  • For 3.7 (or later): Deprecate the cm functions.
  • For 3.9 (or later): Remove the cm functions.

I strongly advocate to introduce this PR in 3.5. If everything goes well, we don't need modifications (only extensions) to the registry, which means that downstream libraries would have 3.5 as a minimum dependency when they switch to the registry.

On read-only colormaps

The registry colormap access already returns copies of the colormaps and by that implements the change to get_cmap, which is scheduled to take effect in 3.6 (#19766). He can be quicker here, because the new API does not have to maintain backward compatibility for the deprecation period.
Other than that, the registry is independent of cmap mutability.

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @timhoffm's path forward. I think additional capabilities can be added later and doesn't preclude this from going in. I think the chosen location at the base level matplotlib.colormaps with a clear name is a good choice.

One other thought (that I don't think is a great idea because it would add another way to do something) is that you could override __getattr__ on this Registry and allow access to the built-ins that way so that
matplotlib.colormaps.viridis is the same as matplotlib.colormaps['viridis']. That may help with moving matplotlib.cm.viridis and friends over to this new location.

@jklymak
Copy link
Member

jklymak commented Aug 14, 2021

My objection is a little more fundamental; I'm not sure why we need a registry at all, so rather than hoisting it to the top level I would tend to discourage passing colormaps by string.

@tacaswell
Copy link
Member

The ship on not supporting colormaps-by-string sailed long ago and I think deprecating that in any meaningful way is out of the question. If we are going to support it, I would rather support it with a nice object which can be both more discover-able and inspect-able than the current state.

@timhoffm
Copy link
Member Author

timhoffm commented Aug 14, 2021

One other thought (that I don't think is a great idea because it would add another way to do something) is that you could override __getattr__ on this Registry and allow access to the built-ins that way so that
matplotlib.colormaps.viridis is the same as matplotlib.colormaps['viridis']. That may help with moving matplotlib.cm.viridis and friends over to this new location.

Thanks for the proposal. It’s always good to add ideas to the solution space. However, I agree that this is not a great idea 😀. As you mentioned, this is just another way to achieve the same thing. And worse: it dilutes the mapping concept. The registry is a simple mapping: name -> Colormap, which is canonically associated with item access. The attributes would pollute the register namespace and introduce an unnecessary asymmetry between built in and third party color maps. I think that matplotlib.cm.viridis was an design flaw in the first place. I don't want to replicate this to the registry. OTOH the future of these cm functions is a separate discussion and possibly targeted at 3.6.

Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>

# workaround: we must defer colormaps import to after loading rcParams, because
# colormap creation depends on rcParams
from matplotlib.cm import _colormaps as colormaps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we handle this in the module level __getattr__ now that we have than in place?

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I leave to @timhoffm 's discretion if we want to add the unregister method in this pass so 👍🏻 self-merging if you want to defer it.

@jklymak
Copy link
Member

jklymak commented Aug 15, 2021

Let's merge. Someone can always add an unregister later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a method to access the list of registered colormaps
6 participants