-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
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? |
You mean as |
I hear but like I have that page of the docs printed out cause of how often I look up the colormaps. |
guess I'm not particularly following the use-case for this versus just |
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 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
|
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 I'd prefer not having to import |
Correct.
It does. The internal
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.)
Agreed. There should be no need for the user to import |
5762928
to
664590a
Compare
👍 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 Or even allow people to set the colormaps just like a dict, but raise on name collisions? |
@greglucas That's exactly the plan. I'm not completely decided on |
@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. |
a19b3ba
to
7f63ce9
Compare
ce7f656
to
79f414a
Compare
Note: I've added It is currently not possible to override builtin colormaps via the To make |
79f414a
to
6049f54
Compare
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.
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
6049f54
to
40f4013
Compare
Docs certainly need adaption. However, I first want to make sure that the API is agreed on before starting to change the docs. |
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 |
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.
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.
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.
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.
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 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): |
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 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.
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 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.
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.
We have unregister_cmap
which went out in 3.4 via #15127
matplotlib/lib/matplotlib/cm.py
Lines 197 to 199 in 1761822
def unregister_cmap(name): | |
""" | |
Remove a colormap recognized by :func:`get_cmap`. |
My sense is that long term we want to deprecate Getting a bit over my skis, having these in a nice object at least opens a path putting |
I do not think we are going to get this done in the next few weeks, I propose bumping it to 3.6. |
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 |
Overall goalThe 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 Target state
Transition pathWhile I would like to eventually remove the
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 colormapsThe registry colormap access already returns copies of the colormaps and by that implements the change to |
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 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.
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. |
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. |
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 |
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.
Could we handle this in the module level __getattr__
now that we have than in place?
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 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.
Let's merge. Someone can always add an unregister later. |
PR Summary
Following the thoughts in #16991 (comment), this defines a Mapping subclass
ColorbarRegistry
with the following properties:Colormap
. - Adding new colormaps is for now still done viaregister_cmap()
. The class will grow the ability to add elements later, but that needs more API consideration.Colormaps
are copies, so that the global state is not affected by modification to the returned colormaps. - This is the goal forget_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? Incm
, inmatplotlib
? Shouldcm
be integrated intocolors
anyway?PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
andpydocstyle<4
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).