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

MAINT deprecated 'spectral' in favor of 'nipy_spectral' #7416

Merged
merged 6 commits into from Nov 10, 2016
Merged

MAINT deprecated 'spectral' in favor of 'nipy_spectral' #7416

merged 6 commits into from Nov 10, 2016

Conversation

NelleV
Copy link
Member

@NelleV NelleV commented Nov 6, 2016

This raises a deprecation warning for the deprecated "spectral" colormap.
I think I covered all the possible calls to "spectral".

closes #7315

@NelleV NelleV added this to the 2.0 (style change major release) milestone Nov 6, 2016
@NelleV
Copy link
Member Author

NelleV commented Nov 6, 2016

@Carreau Do you want to review? :)

@Carreau
Copy link
Contributor

Carreau commented Nov 6, 2016

@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 spectral_r as well (constructed in lib/matplotlib/cm.py)

For post 2.0 / Subsequent PRs:

  • It might be hard, but add a warning filter that turns use of spectral during testing (and doc building ?) into an error.
  • Have keys(), iterkeys(), items() not list spectral and spectral_r

An easy implementation would be to actually remove the data find the dict, and if name==spectral, return super().__getattra__('nipy_spectral_'). But that can be done later.

@Carreau
Copy link
Contributor

Carreau commented Nov 6, 2016

doc/users/image_tutorial.rst uses spectral...

@NelleV
Copy link
Member Author

NelleV commented Nov 6, 2016

Yeah, I forgot to look at the documentation and "fix" it…

@Carreau
Copy link
Contributor

Carreau commented Nov 6, 2016

Sorry I'm slow to review I'm in the train, tethering and the car is shaking quite a bit.

@NelleV
Copy link
Member Author

NelleV commented Nov 7, 2016

Updated it to deprecate spectral_r as well.

@efiring
Copy link
Member

efiring commented Nov 7, 2016

Looks OK to me.

name="spectral",
alternative="nipy_spectral",
obj_type="colormap"
)
Copy link
Contributor

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.

Copy link
Member Author

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()
Copy link
Contributor

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.

Copy link
Member Author

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.

@Carreau
Copy link
Contributor

Carreau commented Nov 7, 2016

Updated it to deprecate spectral_r as well.

+1. Thanks @NelleV !

@NelleV
Copy link
Member Author

NelleV commented Nov 7, 2016

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

@tacaswell
Copy link
Member

Do we want to also deprecate spectral function in pyplot as well?

@NelleV
Copy link
Member Author

NelleV commented Nov 8, 2016

Yes, we should… but how to do this with the function being generated?

@tacaswell
Copy link
Member

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.

@NelleV
Copy link
Member Author

NelleV commented Nov 8, 2016

Got it. I'll do that.

@NelleV
Copy link
Member Author

NelleV commented Nov 9, 2016

Should I run boilerplate and commit the function as well or should we wait for the release?

@efiring
Copy link
Member

efiring commented Nov 9, 2016

Yes, it's best to keep pyplot in sync with everything else.

@tacaswell tacaswell changed the title MAINT deprecated 'spectral' in favor of 'nipy_spectral' [MRG+1] MAINT deprecated 'spectral' in favor of 'nipy_spectral' Nov 9, 2016
@QuLogic
Copy link
Member

QuLogic commented Nov 9, 2016

Wow, pyplot.py seems to be pretty outdated; will have to be careful when backporting.

@tacaswell
Copy link
Member

Should probably just re-run boilerplate after doing the backport.

@efiring efiring merged commit 660aa65 into matplotlib:master Nov 10, 2016
@efiring efiring changed the title [MRG+1] MAINT deprecated 'spectral' in favor of 'nipy_spectral' MAINT deprecated 'spectral' in favor of 'nipy_spectral' Nov 10, 2016
efiring added a commit that referenced this pull request Nov 10, 2016
MAINT deprecated 'spectral' in favor of 'nipy_spectral'

boilerplate.py was run after using 'git cherry-pick -n -m1 660aa65'
@efiring
Copy link
Member

efiring commented Nov 10, 2016

backported as f87100a (including the pyplot.py regenerated by boilerplate.py)

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.

Spectral vs spectral Deprecation warning
5 participants