-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Begin warning on modifying global state of colormaps #16991
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
normalization. | ||
""" | ||
|
||
from collections.abc import MutableMapping | ||
import functools | ||
|
||
import numpy as np | ||
|
@@ -49,7 +50,7 @@ def revcmap(data): | |
LUTSIZE = mpl.rcParams['image.lut'] | ||
|
||
|
||
def _gen_cmap_d(): | ||
def _gen_cmap_registry(): | ||
""" | ||
Generate a dict mapping standard colormap names to standard colormaps, as | ||
well as the reversed colormaps. | ||
|
@@ -65,12 +66,56 @@ def _gen_cmap_d(): | |
# Generate reversed cmaps. | ||
for cmap in list(cmap_d.values()): | ||
rmap = cmap.reversed() | ||
cmap._global = True | ||
rmap._global = True | ||
cmap_d[rmap.name] = rmap | ||
return cmap_d | ||
|
||
|
||
cmap_d = _gen_cmap_d() | ||
locals().update(cmap_d) | ||
class _DeprecatedCmapDictWrapper(MutableMapping): | ||
"""Dictionary mapping for deprecated _cmap_d access.""" | ||
|
||
def __init__(self, cmap_registry): | ||
self._cmap_registry = cmap_registry | ||
|
||
def __delitem__(self, key): | ||
self._warn_deprecated() | ||
self._cmap_registry.__delitem__(key) | ||
greglucas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def __getitem__(self, key): | ||
self._warn_deprecated() | ||
return self._cmap_registry.__getitem__(key) | ||
|
||
def __iter__(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not warn for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oversight, the iter still warns because of the getitem, but I didn't think of people just listing the colormaps and using it manually themselves elsewhere. I just added it here as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, easy enough to fix. Should have just done it myself ;) |
||
self._warn_deprecated() | ||
return self._cmap_registry.__iter__() | ||
|
||
def __len__(self): | ||
self._warn_deprecated() | ||
return self._cmap_registry.__len__() | ||
|
||
def __setitem__(self, key, val): | ||
self._warn_deprecated() | ||
self._cmap_registry.__setitem__(key, val) | ||
|
||
def get(self, key, default=None): | ||
self._warn_deprecated() | ||
return self._cmap_registry.get(key, default) | ||
|
||
def _warn_deprecated(self): | ||
cbook.warn_deprecated( | ||
"3.3", | ||
message="The global colormaps dictionary is no longer " | ||
"considered public API.", | ||
alternative="Please use register_cmap() and get_cmap() to " | ||
"access the contents of the dictionary." | ||
) | ||
|
||
|
||
_cmap_registry = _gen_cmap_registry() | ||
locals().update(_cmap_registry) | ||
# This is no longer considered public API | ||
cmap_d = _DeprecatedCmapDictWrapper(_cmap_registry) | ||
|
||
|
||
# Continue with definitions ... | ||
|
@@ -95,6 +140,13 @@ def register_cmap(name=None, cmap=None, data=None, lut=None): | |
and the resulting colormap is registered. Instead of this implicit | ||
colormap creation, create a `.LinearSegmentedColormap` and use the first | ||
case: ``register_cmap(cmap=LinearSegmentedColormap(name, data, lut))``. | ||
|
||
Notes | ||
----- | ||
Registering a colormap stores a reference to the colormap object | ||
which can currently be modified and inadvertantly change the global | ||
colormap state. This behavior is deprecated and in Matplotlib 3.5 | ||
the registered colormap will be immutable. | ||
""" | ||
cbook._check_isinstance((str, None), name=name) | ||
if name is None: | ||
|
@@ -104,7 +156,8 @@ def register_cmap(name=None, cmap=None, data=None, lut=None): | |
raise ValueError("Arguments must include a name or a " | ||
"Colormap") from err | ||
if isinstance(cmap, colors.Colormap): | ||
cmap_d[name] = cmap | ||
cmap._global = True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please document in the docstring that if passing a Colormap instance, that instance will become immutable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added in a Notes section to both the |
||
_cmap_registry[name] = cmap | ||
return | ||
if lut is not None or data is not None: | ||
cbook.warn_deprecated( | ||
|
@@ -117,7 +170,8 @@ def register_cmap(name=None, cmap=None, data=None, lut=None): | |
if lut is None: | ||
lut = mpl.rcParams['image.lut'] | ||
cmap = colors.LinearSegmentedColormap(name, data, lut) | ||
cmap_d[name] = cmap | ||
cmap._global = True | ||
_cmap_registry[name] = cmap | ||
|
||
|
||
def get_cmap(name=None, lut=None): | ||
|
@@ -127,11 +181,17 @@ def get_cmap(name=None, lut=None): | |
Colormaps added with :func:`register_cmap` take precedence over | ||
built-in colormaps. | ||
|
||
Notes | ||
----- | ||
Currently, this returns the global colormap object, which is deprecated. | ||
In Matplotlib 3.5, you will no longer be able to modify the global | ||
colormaps in-place. | ||
|
||
Parameters | ||
---------- | ||
name : `matplotlib.colors.Colormap` or str or None, default: None | ||
If a `.Colormap` instance, it will be returned. Otherwise, the name of | ||
a colormap known to Matplotlib, which will be resampled by *lut*. The | ||
If a `.Colormap` instance, it will be returned. Otherwise, the name of | ||
a colormap known to Matplotlib, which will be resampled by *lut*. The | ||
default, None, means :rc:`image.cmap`. | ||
lut : int or None, default: None | ||
If *name* is not already a Colormap instance and *lut* is not None, the | ||
|
@@ -141,11 +201,11 @@ def get_cmap(name=None, lut=None): | |
name = mpl.rcParams['image.cmap'] | ||
if isinstance(name, colors.Colormap): | ||
return name | ||
cbook._check_in_list(sorted(cmap_d), name=name) | ||
cbook._check_in_list(sorted(_cmap_registry), name=name) | ||
if lut is None: | ||
return cmap_d[name] | ||
return _cmap_registry[name] | ||
else: | ||
return cmap_d[name]._resample(lut) | ||
return _cmap_registry[name]._resample(lut) | ||
|
||
|
||
class ScalarMappable: | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
Just as a remark (not necessarily to be acted upon in this PR):
By making the cmap registry private, we don't have an official way to get the registered colormaps from the
cm
module, where I would expect such functionality. This is something we should take care of in followup PRs.I'm not even sure
plt.colormaps()
is needed. First, It's essentially a lengthy description of colormaps, which should be somewhere in the docs but not in a pyplot docstring. Second, I don't see providing the colormaps as a core functionality of pyplot, but since there's a lot of historic colormap stuff in pyplot, that's maybe ok.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 had the same thought: #16991 (comment)
I'm not sure what the best step is to expose the access, but would agree that I don't want to have to go to pyplot if not necessary. Maybe rather than a
cmap_d
, we could do something like your MutableMapping suggestion and then override the get/set methods to ensure copy/immutable return.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.
Sorry if this is a bit unstructured. I just stated typing and ideas unfolded along the way.
There would be two ways:
The
cm
module currently uses simple functionsget_cmap()
andregister_cmap()
. In that spirit, you could add a functionget_cmap_names()
(naming t.b.d.) but we have to make a distinction betweenget_cmap
returning an instance and that function (probably?) returning strings.The alternative would be to expose a
ColormapRegistry
object, which provides all necessary functionality. That would likely be subclassingMutableMapping
to look dict-like but still perform all relevant checks (maybe it would alternatively only implementMapping
and provide an explicit register method if we feel the mutation should be more special (i.e. we don't wantdel registry['jet']
orregistry['jet'] = Colormap(...)
).This would also replace the module level functions; at least their implementation. Whether they will additionally stay for backward compatibility is a separate topic.
In the end 2 is the more complex approach. However, I have a slight preference for it because it properly encapsulates the colormap handling. On the downside it changes the API from calling simple cm functions to an object. But then again, we could move the actual instance to a variable
matplotlib.colormaps
, which may make more sense and users would usually not need to import matplotlib.cm anymore; andpyplot.colormaps
could later even point to the same instance (with some API compatibility because the keys of that mapping would then colormap names, sofor name in plt.colormaps()
would still work.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 pretty much everything you're saying. I also don't think 2 would be that hard to implement. An issue with naming will be found with: #11413 which is already proposing to rename cm to colormaps. This discussion is really more a part of the MEP: https://matplotlib.org/3.2.1/devel/MEP/MEP21.html
Maybe we should do some larger work on an overarching vision there and then decide on how best to get there?
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.
If we provide colormap registry access from
matplotlib
directly,cm.get_cmap
andcm.register_cmap
become obsolete. There won't be a need for the end user to importcm
, which in turn reduces the need for renaming (#11413). But we could still find other names for that.It would be reasonable to detail the overall vision.