-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
MAINT deprecated 'spectral' in favor of 'nipy_spectral' #7416
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
Conversation
@Carreau Do you want to review? :) |
Yes :-) The name vas changed in 2012, so I would not be shy as marking as deprecated since at least matplotlib 1.5. You might want to warn on For post 2.0 / Subsequent PRs:
An easy implementation would be to actually remove the data find the dict, and if name==spectral, return |
|
Yeah, I forgot to look at the documentation and "fix" it… |
Sorry I'm slow to review I'm in the train, tethering and the car is shaking quite a bit. |
Updated it to deprecate |
Looks OK to me. |
name="spectral", | ||
alternative="nipy_spectral", | ||
obj_type="colormap" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to make the message dynamic for spectral_r
as well ? I can see pros and cons to both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to be lazy, and not do it, but… maybe I should.
from matplotlib._cm import cubehelix | ||
from matplotlib._cm_listed import cmaps as cmaps_listed | ||
|
||
cmap_d = dict() | ||
cmap_d = _deprecation_datad() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that line 88 of this file will likely trigger the deprecation warning if I'm not wrong, as it loop through all the colormap to build the reversed ones. Not sure what we can do about the though. Just pointing it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argh… you are right.
I'll fix that.
+1. Thanks @NelleV ! |
I'm now catching warnings when creating the reverse colormaps to avoid raising the deprecation warning when cm.py is imported (ie, when anyone uses matplotlib). |
Do we want to also deprecate |
Yes, we should… but how to do this with the function being generated? |
https://github.com/matplotlib/matplotlib/blob/v2.x/boilerplate.py#L333 split the list into two (deprecated and not) and make a new template which includes the deprecation warning. |
Got it. I'll do that. |
The documentation now uses nipy_spectral
We don't want to raise the deprecation warning for spectral/spectral_r unless the user calls that colormap.
Should I run boilerplate and commit the function as well or should we wait for the release? |
Yes, it's best to keep pyplot in sync with everything else. |
Wow, |
Should probably just re-run boilerplate after doing the backport. |
MAINT deprecated 'spectral' in favor of 'nipy_spectral' boilerplate.py was run after using 'git cherry-pick -n -m1 660aa65'
backported as f87100a (including the pyplot.py regenerated by boilerplate.py) |
…th matplotlib>=2.2 See: matplotlib/matplotlib#7416
This raises a deprecation warning for the deprecated "spectral" colormap.
I think I covered all the possible calls to "spectral".
closes #7315