Skip to content

Changing get_cmap to return copies of the registered colormaps. #16943

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Changing get_cmap to return copies of the registered colormaps. #16943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
333bc06
Changing get_cmap to return copies of the registered colormaps.
greglucas Mar 28, 2020
56fc4ef
Adding a deprecation warning period for get_cmap().
greglucas Mar 29, 2020
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
14 changes: 11 additions & 3 deletions doc/api/next_api_changes/behaviour.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,9 @@ deprecation warning.
`~.Axes.errorbar` now color cycles when only errorbar color is set
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Previously setting the *ecolor* would turn off automatic color cycling for the plot, leading to the
the lines and markers defaulting to whatever the first color in the color cycle was in the case of
multiple plot calls.
Previously setting the *ecolor* would turn off automatic color cycling for the plot, leading to the
the lines and markers defaulting to whatever the first color in the color cycle was in the case of
multiple plot calls.

`.rcsetup.validate_color_for_prop_cycle` now always raises TypeError for bytes input
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down Expand Up @@ -147,3 +147,11 @@ The parameter ``s`` to `.Axes.annotate` and `.pyplot.annotate` is renamed to
The old parameter name remains supported, but
support for it will be dropped in a future Matplotlib release.

``get_cmap()`` now returns a copy of the colormap
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Previously, calling ``get_cmap()`` would return
the built-in Colormap. If you made modifications to that colormap, the
changes would be propagated in the global state. This function now
returns a copy of all registered colormaps to keep the built-in
Copy link
Member

Choose a reason for hiding this comment

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

"... copy of the requested colormap..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this even go into this now if it is a future change and there are not currently any behavior changes?

My thoughts are to actually break these commits up into two separate PRs. One with the deprecation warning/preparation for a future version. Then another one with the proposed copy code in it that could be pushed off for quite a while and discussed for immutability or not.

Copy link
Member

Choose a reason for hiding this comment

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

I personally think the globals should be immutable. If folks are modifying the global to then call again as a named colormap, then I think they were using a misfeature/bug.

I just gave a comment here because I was reading it, and had a quick thought. Please work out w/ Tim Hoffman how to do the rest of the PR, but this seems to be the right direction to be heading.

colormaps untouched.

31 changes: 25 additions & 6 deletions lib/matplotlib/cm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
normalization.
"""

import copy
import functools

import numpy as np
Expand Down Expand Up @@ -70,9 +71,9 @@ def _gen_cmap_d():


cmap_d = _gen_cmap_d()
# locals().update(copy.deepcopy(cmap_d))
locals().update(cmap_d)


# Continue with definitions ...


Expand All @@ -95,6 +96,9 @@ 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))``.

If *name* is the same as a built-in colormap this will replace the
built-in Colormap of the same name.
"""
cbook._check_isinstance((str, None), name=name)
if name is None:
Expand All @@ -105,6 +109,8 @@ def register_cmap(name=None, cmap=None, data=None, lut=None):
"Colormap") from err
if isinstance(cmap, colors.Colormap):
cmap_d[name] = cmap
# We are overriding the global state, so reinitialize this.
cmap._global_changed = False
return
if lut is not None or data is not None:
cbook.warn_deprecated(
Expand All @@ -118,21 +124,24 @@ def register_cmap(name=None, cmap=None, data=None, lut=None):
lut = mpl.rcParams['image.lut']
cmap = colors.LinearSegmentedColormap(name, data, lut)
cmap_d[name] = cmap
cmap._global_changed = False


def get_cmap(name=None, lut=None):
"""
Get a colormap instance, defaulting to rc values if *name* is None.

Colormaps added with :func:`register_cmap` take precedence over
built-in colormaps.
Colormaps added with :func:`register_cmap` with the same name as
built-in colormaps will replace them.

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`.
If a `.Colormap` instance, it will be returned.
Otherwise, the name of a colormap known to Matplotlib, which will
be resampled by *lut*. Currently, this returns the global colormap
instance which is deprecated. In Matplotlib 4, a copy of the requested
Colormap will be returned. 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
colormap will be resampled to have *lut* entries in the lookup table.
Expand All @@ -143,7 +152,17 @@ def get_cmap(name=None, lut=None):
return name
cbook._check_in_list(sorted(cmap_d), name=name)
if lut is None:
if cmap_d[name]._global_changed:
cbook.warn_deprecated(
"3.3",
message="The colormap requested has had the global state "
"changed without being registered. Accessing a "
"colormap in this way has been deprecated. "
"Please register the colormap using "
"plt.register_cmap() before requesting "
"the modified colormap.")
Copy link
Member

@jklymak jklymak Mar 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I understand this, but only because I read the thread. If I was a user who ran

plt.get_cmap('viridis').set_bad('k')
fig, ax = plt.subplots()
ax.imshow(data, cmap='viridis')

I'd have no idea what I was supposed to do here. I also don't think we should be encouraging any changing of the global state. I'd far rather tell folks that they should deal with the cmap object which is far less arcane than registering the global state:

cmp = plt.get_cmap('viridis').set_bad('k')
fig, ax = plt.subplots()
ax.imshow(data, cmap=cmp)

To that end, I'd say "Modifying a registered colormap is deprecated; make a new copy via cmp=plt.get_cmap('viridian') and modify the copy." ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, that won't even work either! (which is why I think this is a bug) You would need to cmp=copy.copy(plt.get_cmap('viridian')) currently to get the desired functionality. The real problem is that further down if you go and request 'viridis' from a plot after doing a bunch of stuff, you will still get the bad colormap.

return cmap_d[name]
# return copy.copy(cmap_d[name])
else:
return cmap_d[name]._resample(lut)

Expand Down
19 changes: 19 additions & 0 deletions lib/matplotlib/colors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,6 +515,8 @@ def __init__(self, name, N=256):
self._i_over = self.N + 1
self._i_bad = self.N + 2
self._isinit = False
# This is to aid in deprecation transition for v3.3
self._global_changed = False

#: When this colormap exists on a scalar mappable and colorbar_extend
#: is not False, colorbar creation will pick up ``colorbar_extend`` as
Expand Down Expand Up @@ -599,13 +601,27 @@ def __copy__(self):
cmapobject.__dict__.update(self.__dict__)
if self._isinit:
cmapobject._lut = np.copy(self._lut)
cmapobject._global_changed = False
return cmapobject

def __eq__(self, other):
if isinstance(other, Colormap):
# To compare lookup tables the Colormaps have to be initialized
if not self._isinit:
self._init()
if not other._isinit:
other._init()
if self._lut.shape != other._lut.shape:
return False
return np.all(self._lut == other._lut)
return False

def set_bad(self, color='k', alpha=None):
"""Set the color for masked values."""
self._rgba_bad = to_rgba(color, alpha)
if self._isinit:
self._set_extremes()
self._global_changed = True

def set_under(self, color='k', alpha=None):
"""
Expand All @@ -614,6 +630,7 @@ def set_under(self, color='k', alpha=None):
self._rgba_under = to_rgba(color, alpha)
if self._isinit:
self._set_extremes()
self._global_changed = True

def set_over(self, color='k', alpha=None):
"""
Expand All @@ -622,6 +639,7 @@ def set_over(self, color='k', alpha=None):
self._rgba_over = to_rgba(color, alpha)
if self._isinit:
self._set_extremes()
self._global_changed = True

def _set_extremes(self):
if self._rgba_under:
Expand Down Expand Up @@ -2098,6 +2116,7 @@ def from_levels_and_colors(levels, colors, extend='neither'):
cmap.set_over('none')

cmap.colorbar_extend = extend
cmap._global_changed = False

norm = BoundaryNorm(levels, ncolors=n_data_colors)
return cmap, norm
32 changes: 31 additions & 1 deletion lib/matplotlib/tests/test_colors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def test_resample():


def test_register_cmap():
new_cm = copy.copy(plt.cm.viridis)
new_cm = plt.get_cmap('viridis')
cm.register_cmap('viridis2', new_cm)
assert plt.get_cmap('viridis2') == new_cm

Expand All @@ -70,6 +70,36 @@ def test_register_cmap():
cm.register_cmap()


@pytest.mark.skipif(matplotlib.__version__[0] < "4",
reason="This test modifies the global state of colormaps.")
def test_colormap_builtin_immutable():
new_cm = plt.get_cmap('viridis')
new_cm.set_over('b')
# Make sure that this didn't mess with the original viridis cmap
assert new_cm != plt.get_cmap('viridis')

# Also test that pyplot access doesn't mess the original up
new_cm = plt.cm.viridis
new_cm.set_over('b')
assert new_cm != plt.get_cmap('viridis')


def test_colormap_builtin_immutable_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="The colormap requested has had the global"):
new_cm.set_under('k')
# This should warn now because we've modified the global state
# without registering it
plt.get_cmap('viridis')

# Test that re-registering the original cmap clears the warning
plt.register_cmap(cmap=orig_cmap)
plt.get_cmap('viridis')


def test_colormap_copy():
cm = plt.cm.Reds
cm_copy = copy.copy(cm)
Expand Down