Skip to content
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

Fix matplotlib colormaps not loading properly #319

Merged
merged 3 commits into from Apr 18, 2016

Conversation

pllim
Copy link
Collaborator

@pllim pllim commented Apr 7, 2016

I found a bug where matplotlib colormaps were not being loaded even if requested in Python 2.7. It is because the colormap names were compared to str when they are actually unicode.

While I am at it, I also made sure the hot new "viridis" colormap is present:

screenshot

Also added some much needed docstrings.

# See http://matplotlib.org/style_changes.html
if 'viridis' not in names:
names.append('viridis')
names.sort()
Copy link
Owner

Choose a reason for hiding this comment

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

Can we be sure 'viridis' was loaded? Does this work with e.g. older matplotlib versions. I'm not sure how old we should support, but maybe something like 1.3.1?

Copy link
Collaborator Author

@pllim pllim Apr 18, 2016

Choose a reason for hiding this comment

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

Good point. Would changing

cm = plt.get_cmap(name)
try:
    add_matplotlib_cmap(cm, name=name)
except Exception as e:
    ...

to

try:
    cm = plt.get_cmap(name)
    add_matplotlib_cmap(cm, name=name)
except Exception as e:
    ...

be sufficient to address your concern?

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, I guess it works because if it is not present it gets thrown out in the exception handler below. @pllim, did you find it necessary to add this line to get 'viridis' to load with newer matplotlib versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have 1.5.1, which I think is sufficiently new. I need that line to find Viridis. For whatever reason, it does not get included automatically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can double check to see if it is in plt.cm.datad.keys() in your own Matplotlib installation. If you find otherwise, please let me know, because then it could be something wrong with my Matplotlib installation.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm fine with leaving the line in, but maybe we should add a comment to the effect that the line is there because the colormap is somehow not picked up automatically with "transition" versions of matplotlib (or something to that effect).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I'll add that comment in this PR when I get the chance. Thanks!

@pllim
Copy link
Collaborator Author

pllim commented Apr 18, 2016

Anyways, I also opened an issue with Matplotlib people about Viridis missing in the keys (See matplotlib/matplotlib#6316).

…in case Viridis not in Matplotlib. Added a comment about Viridis.
@pllim
Copy link
Collaborator Author

pllim commented Apr 18, 2016

@ejeschke , I think I addressed your concerns in my third commit to this PR. Please let me know if it is sufficient. Thanks!

@ejeschke ejeschke merged commit fb9c395 into ejeschke:master Apr 18, 2016
@ejeschke
Copy link
Owner

Looks good!

@ejeschke
Copy link
Owner

Regarding the "viridis" map, on matplotlib 1.3.1 I now get a prominent print message "Error adding colormap 'viridis': Colormap viridis is not recognized" when starting up the reference viewer. I think that the code adding this name also needs to check whether this color map is actually available...

@pllim
Copy link
Collaborator Author

pllim commented Apr 19, 2016

Yikes, sorry for the trouble. I'll investigate a way to avoid a print out error tomorrow. One way is to only enable it for Matplotlib 1.5+ if I cannot find something more elegant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants