Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -38,7 +38,7 @@ | ||
a = np.vstack((a, a)) | ||
# List of all colormaps; skip reversed colormaps. | ||
maps = sorted(m for m in plt.colormaps() if not m.endswith("_r")) | ||
Member 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. 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 I'm not even sure 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 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 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. Sorry if this is a bit unstructured. I just stated typing and ideas unfolded along the way. There would be two ways:
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 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 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 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. If we provide colormap registry access from It would be reasonable to detail the overall vision. | ||
ncol = 2 | ||
nrow = len(maps)//ncol + 1 | ||
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_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 | ||
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 resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
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._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._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 | ||
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_registry), name=name) | ||
if lut is None: | ||
return_cmap_registry[name] | ||
else: | ||
return_cmap_registry[name]._resample(lut) | ||
class ScalarMappable: | ||