Skip to content

Ensure name in set_cmap is contained in cmap_d #17012

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

Ensure name in set_cmap is contained in cmap_d #17012

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0abb3c
modify set_cmap to always use the correct cmap name
ianhi Apr 3, 2020
cb02f11
flake8 changes
ianhi Apr 3, 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: 10 additions & 4 deletions lib/matplotlib/pyplot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
import numpy as np

# We may not need the following imports here:
from matplotlib.colors import Normalize
from matplotlib.colors import Normalize, Colormap
from matplotlib.lines import Line2D
from matplotlib.text import Text, Annotation
from matplotlib.patches import Polygon, Rectangle, Circle, Arrow
Expand Down Expand Up @@ -2069,11 +2069,17 @@ def set_cmap(cmap):
matplotlib.cm.register_cmap
matplotlib.cm.get_cmap
"""
cmap = cm.get_cmap(cmap)
cbook._check_isinstance((str, Colormap), cmap=cmap)
if isinstance(cmap, str):
name = cmap
cmap = get_cmap(name)
rc('image', cmap=name)
elif isinstance(cmap, Colormap):
if cmap.name not in cm.cmap_d.keys():
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the .keys() when checking if something is in a dictionary.

register_cmap(cmap.name, cmap)
rc('image', cmap=cmap.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you could get into another tricky situation.

Say someone has already registered a different Colormap under the same name. So, the cmap.name is in the dictionary already, but it is actually pointing to a different colormap and this wouldn't set the global cmap to what you're expecting either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this is tricky. Perhaps:

if cmap.name not in cm.cmap_d:
    register_cmap(cmap.name, cmap)
elif cmap is not get_cmap(cmap.name):
    raise some sort of error/warning

and in that error/warning explain that they should use register_cmap or change the name attribute of their colormap?

Also good to note that as originally written this function falls into the same trap. Though I suppose it's not super desirable to maintain backwards compatibility with a silent bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, something like this would be desirable going forward to make it a little more clear what is going on and raising rather than silently doing something unexpected!


rc('image', cmap=cmap.name)
im = gci()

if im is not None:
im.set_cmap(cmap)

Expand Down