Skip to content

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

Merged
merged 2 commits into from
Apr 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/images_contours_and_fields/demo_bboximage.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
a = np.vstack((a, a))

# List of all colormaps; skip reversed colormaps.
maps = sorted(m for m in plt.cm.cmap_d if not m.endswith("_r"))
maps = sorted(m for m in plt.colormaps() if not m.endswith("_r"))
Copy link
Member

@timhoffm timhoffm Apr 26, 2020

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.

Copy link
Contributor Author

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.

Copy link
Member

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:

  1. The cm module currently uses simple functions get_cmap() and register_cmap(). In that spirit, you could add a function get_cmap_names() (naming t.b.d.) but we have to make a distinction between get_cmap returning an instance and that function (probably?) returning strings.

  2. The alternative would be to expose a ColormapRegistry object, which provides all necessary functionality. That would likely be subclassing MutableMapping to look dict-like but still perform all relevant checks (maybe it would alternatively only implement Mapping and provide an explicit register method if we feel the mutation should be more special (i.e. we don't want del registry['jet'] or registry['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; and pyplot.colormaps could later even point to the same instance (with some API compatibility because the keys of that mapping would then colormap names, so for name in plt.colormaps() would still work.

Copy link
Contributor Author

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?

Copy link
Member

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 and cm.register_cmap become obsolete. There won't be a need for the end user to import cm, 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.


ncol = 2
nrow = len(maps)//ncol + 1
Expand Down
4 changes: 2 additions & 2 deletions lib/matplotlib/backends/qt_editor/figureoptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,11 @@ def prepare_data(d, init):
mappabledict[label] = mappable
mappablelabels = sorted(mappabledict, key=cmp_key)
mappables = []
cmaps = [(cmap, name) for name, cmap in sorted(cm.cmap_d.items())]
cmaps = [(cmap, name) for name, cmap in sorted(cm._cmap_registry.items())]
for label in mappablelabels:
mappable = mappabledict[label]
cmap = mappable.get_cmap()
if cmap not in cm.cmap_d.values():
if cmap not in cm._cmap_registry.values():
cmaps = [(cmap, cmap.name), *cmaps]
low, high = mappable.get_clim()
mappabledata = [
Expand Down
80 changes: 70 additions & 10 deletions lib/matplotlib/cm.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
normalization.
"""

from collections.abc import MutableMapping
import functools

import numpy as np
Expand Down Expand Up @@ -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.
Expand All @@ -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)

def __getitem__(self, key):
self._warn_deprecated()
return self._cmap_registry.__getitem__(key)

def __iter__(self):
Copy link
Member

Choose a reason for hiding this comment

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

This does not warn for list(mpl.cm.cmap_d) but does warn for list(mlp.cm.cmap_d.items()). Was this an oversight that __iter__ does not warn or a decision that because it is only giving a list of keys that could then go to get_cmap it should not wark?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

same for __len__ below? Sorry for the thousand paper cuts at the end...

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ...
Expand All @@ -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:
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added in a Notes section to both the get_cmap and register_cmap functions now. Let me know if you want any different wording there. It may be odd to talk about future behavior there considering you can't do anything to get that future behavior yet? But I can't think of any great wording off the top of my head.

_cmap_registry[name] = cmap
return
if lut is not None or data is not None:
cbook.warn_deprecated(
Expand All @@ -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):
Expand All @@ -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
Expand All @@ -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:
Expand Down
16 changes: 16 additions & 0 deletions lib/matplotlib/colors.py
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,18 @@ def makeMappingArray(N, data, gamma=1.0):
return _create_lookup_table(N, data, gamma)


def _warn_if_global_cmap_modified(cmap):
if getattr(cmap, '_global', False):
cbook.warn_deprecated(
"3.3",
message="You are modifying the state of a globally registered "
"colormap. In future versions, you will not be able to "
"modify a registered colormap in-place. To remove this "
"warning, you can make a copy of the colormap first. "
f"cmap = mpl.cm.get_cmap({cmap.name}).copy()"
)


class Colormap:
"""
Baseclass for all scalar to RGBA mappings.
Expand Down Expand Up @@ -599,10 +611,12 @@ def __copy__(self):
cmapobject.__dict__.update(self.__dict__)
if self._isinit:
cmapobject._lut = np.copy(self._lut)
cmapobject._global = False
return cmapobject

def set_bad(self, color='k', alpha=None):
"""Set the color for masked values."""
_warn_if_global_cmap_modified(self)
self._rgba_bad = to_rgba(color, alpha)
if self._isinit:
self._set_extremes()
Expand All @@ -611,6 +625,7 @@ def set_under(self, color='k', alpha=None):
"""
Set the color for low out-of-range values when ``norm.clip = False``.
"""
_warn_if_global_cmap_modified(self)
self._rgba_under = to_rgba(color, alpha)
if self._isinit:
self._set_extremes()
Expand All @@ -619,6 +634,7 @@ def set_over(self, color='k', alpha=None):
"""
Set the color for high out-of-range values when ``norm.clip = False``.
"""
_warn_if_global_cmap_modified(self)
self._rgba_over = to_rgba(color, alpha)
if self._isinit:
self._set_extremes()
Expand Down
2 changes: 1 addition & 1 deletion lib/matplotlib/pyplot.py
Original file line number Diff line number Diff line change
Expand Up @@ -1948,7 +1948,7 @@ def colormaps():
<https://www.mathworks.com/matlabcentral/fileexchange/2662-cmrmap-m>`_
by Carey Rappaport
"""
return sorted(cm.cmap_d)
return sorted(cm._cmap_registry)


def _setup_pyplot_info_docstrings():
Expand Down
37 changes: 36 additions & 1 deletion lib/matplotlib/tests/test_colors.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,41 @@ def test_register_cmap():
cm.register_cmap()


def test_colormap_global_set_warn():
new_cm = plt.get_cmap('viridis')
# Store the old value so we don't override the state later on.
orig_cmap = copy.copy(new_cm)
with pytest.warns(cbook.MatplotlibDeprecationWarning,
match="You are modifying the state of a globally"):
# This should warn now because we've modified the global state
new_cm.set_under('k')

# This shouldn't warn because it is a copy
copy.copy(new_cm).set_under('b')

# Test that registering and then modifying warns
plt.register_cmap(name='test_cm', cmap=copy.copy(orig_cmap))
new_cm = plt.get_cmap('test_cm')
with pytest.warns(cbook.MatplotlibDeprecationWarning,
match="You are modifying the state of a globally"):
# This should warn now because we've modified the global state
new_cm.set_under('k')

# Re-register the original
plt.register_cmap(cmap=orig_cmap)


def test_colormap_dict_deprecate():
# Make sure we warn on get and set access into cmap_d
with pytest.warns(cbook.MatplotlibDeprecationWarning,
match="The global colormaps dictionary is no longer"):
cm = plt.cm.cmap_d['viridis']

with pytest.warns(cbook.MatplotlibDeprecationWarning,
match="The global colormaps dictionary is no longer"):
plt.cm.cmap_d['test'] = cm


def test_colormap_copy():
cm = plt.cm.Reds
cm_copy = copy.copy(cm)
Expand Down Expand Up @@ -818,7 +853,7 @@ def test_pandas_iterable(pd):
assert_array_equal(cm1.colors, cm2.colors)


@pytest.mark.parametrize('name', sorted(cm.cmap_d))
@pytest.mark.parametrize('name', sorted(plt.colormaps()))
def test_colormap_reversing(name):
"""
Check the generated _lut data of a colormap and corresponding reversed
Expand Down
2 changes: 1 addition & 1 deletion lib/matplotlib/tests/test_pickle.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ def test_shared():
assert fig.axes[1].get_xlim() == (10, 20)


@pytest.mark.parametrize("cmap", cm.cmap_d.values())
@pytest.mark.parametrize("cmap", cm._cmap_registry.values())
def test_cmap(cmap):
pickle.dumps(cmap)

Expand Down