Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
jklymak merged 2 commits intomatplotlib:masterfromtimhoffm:colormap-registry
Aug 15, 2021

Conversation

timhoffm
Copy link
Member

PR Summary

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

  • read-only mapping: name ->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.
  • The returnedColormaps 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 instancecolormaps instance live? Incm, inmatplotlib? Shouldcm be integrated intocolors anyway?

PR Checklist

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (runflake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • Conforms to Matplotlib style conventions (installflake8-docstrings andpydocstyle<4 and runflake8 --docstring-convention=all).
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented indoc/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
MemberAuthor

You mean asColormapRegistry.__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

guess I'm not particularly following the use-case for this versus justget_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
MemberAuthor

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 functionsget_cmap,register_cmap working on that. By making the dict private, users lost access to the keys.

Youcould 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 ofcm. 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 mplmpl.colormaps.register(my_colormap)cmap = mpl.colormaps['jet']

@jklymak
Copy link
Member

OK, so this is just a first step to this class being a new interface for the colormap registry. Because right now, if I usecmap_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 importcm to get this list. I always importpyplot, 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
MemberAuthor

timhoffm commentedSep 17, 2020
edited
Loading

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 usecmap_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 internalself._cmaps is a reference to the same object as_cmap_registry:

In [9]: cm.colormaps._cmaps is cm._cmap_registryOut[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 importcm to get this list. I always importpyplot, 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 importcm. The nice thing with an object is that we can easily reference it from anywhere. We can instantiate it incm but dofrom matplotlib.cm import colormaps inmatplotlib andpyplot (or we directly instantiate it inmatplotlib and only import inpyplot.

@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 aregister() 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

story645 reacted with thumbs up emoji

@timhoffm
Copy link
MemberAuthor

timhoffm commentedSep 20, 2020
edited
Loading

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

@jklymakjklymak added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelOct 21, 2020
@jklymakjklymak removed the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelJan 5, 2021
@QuLogicQuLogic modified the milestones:v3.4.0,v3.5.0Jan 21, 2021
@jklymakjklymak mentioned this pull requestMar 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.

@timhoffmtimhoffmforce-pushed thecolormap-registry branch 5 times, most recently froma19b3ba to7f63ce9CompareMay 14, 2021 01:20
@timhoffmtimhoffm marked this pull request as ready for reviewAugust 1, 2021 22:55
@timhoffm
Copy link
MemberAuthor

timhoffm commentedAug 1, 2021
edited
Loading

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

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

To makempl.colormaps work, I had to do a late import ofcm because that depends onrcParams 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.

Copy link
Contributor

@greglucasgreglucas 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 usempl.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
Copy link
MemberAuthor

timhoffm commentedAug 5, 2021
edited
Loading

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.

greglucas reacted with thumbs up emoji

@jklymak
Copy link
Member

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
MemberAuthor

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 privateColormapRegistry._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 notmean 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? withforce 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
MemberAuthor

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 haveunregister_cmap which went out in 3.4 via#15127

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

@tacaswell
Copy link
Member

My sense is that long term we want to deprecatempl.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 puttingfilter(...) 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 aboutmpl.cm.{cmap_name}.

@timhoffm
Copy link
MemberAuthor

timhoffm commentedAug 14, 2021
edited
Loading

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 likeget_cmap(.. lut=...) or the parameter orderregister_cmap(name, cmap) where name is optional.

Target state

  • thematplotlib.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 thematplotlib.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 thecm 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 thecm functions.
  • For 3.9 (or later): Remove thecm 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 toget_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

@greglucasgreglucas 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 levelmatplotlib.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 asmatplotlib.colormaps['viridis']. That may help with movingmatplotlib.cm.viridis and friends over to this new location.

@jklymak
Copy link
Member

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.

jklymak and timhoffm reacted with thumbs up emoji

@timhoffm
Copy link
MemberAuthor

timhoffm commentedAug 14, 2021
edited
Loading

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 asmatplotlib.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

@tacaswelltacaswell 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

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell approved these changes

@greglucasgreglucasgreglucas approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

Add a method to access the list of registered colormaps
6 participants
@timhoffm@story645@jklymak@greglucas@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp