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

Deprecate the Accent colormap, in anticipation of changes in MPL 2.0 #1193

Merged
merged 5 commits into from Mar 14, 2017

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented Mar 13, 2017

Addresses #1188

@matthew-brett

Thanks for cleaning that one up.

@@ -362,6 +362,9 @@ def mrtrix_spherical_functions():
def get_cmap(name):
"""Makes a callable, similar to maptlotlib.pyplot.get_cmap"""
if name.lower() == "accent":
raise ValueError("The `Accent` colormap is deprecated as of version",

This comment has been minimized.

@matthew-brett

matthew-brett Mar 13, 2017

Member

Not deprecating though - seems a bit much to go straight to error.

@@ -109,14 +109,17 @@ def test_colormap():
@npt.dec.skipif(not fvtk.have_matplotlib)
def test_colormaps_matplotlib():
v = np.random.random(1000)
for name in 'jet', 'Blues', 'Accent', 'bone':
for name in 'jet', 'Blues', 'bone':

This comment has been minimized.

@matthew-brett

matthew-brett Mar 13, 2017

Member

I guess you add make the accent test conditional on mpl < 2.

This comment has been minimized.

@arokem

arokem Mar 13, 2017

Member

On further thought, I think we should deprecate this colormap, regardless of the version of MPL that the user happens to have installed. The reasons they have for changing it into a linear segmented color map apply to us as well, and we shouldn't be using it as a continuous color map at all.

This comment has been minimized.

@matthew-brett

matthew-brett Mar 13, 2017

Member

Sure, I agree about deprecation regardless, just suggesting testing the colormap to make sure it's correct, even if deprecated.

@coveralls

This comment has been minimized.

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.001%) to 88.423% when pulling b914325 on arokem:deprecate-accent into 32ed56b on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Mar 13, 2017

Codecov Report

Merging #1193 into master will increase coverage by <.01%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
+ Coverage   85.93%   85.93%   +<.01%     
==========================================
  Files         219      219              
  Lines       26383    26399      +16     
  Branches     2706     2709       +3     
==========================================
+ Hits        22671    22685      +14     
+ Misses       3052     3050       -2     
- Partials      660      664       +4
Impacted Files Coverage Δ
dipy/data/init.py 89.54% <100%> (+0.2%)
dipy/viz/tests/test_fvtk.py 92.4% <85.71%> (-1.54%)
dipy/utils/six.py 45.71% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32ed56b...5d83559. Read the comment docs.

@@ -362,6 +362,10 @@ def mrtrix_spherical_functions():
def get_cmap(name):
"""Makes a callable, similar to maptlotlib.pyplot.get_cmap"""
if name.lower() == "accent":
warnings.warn("The `Accent` colormap is deprecated as of version" +

This comment has been minimized.

@matthew-brett

matthew-brett Mar 13, 2017

Member

FutureWarning ? DeprecationWarning?

This comment has been minimized.

@arokem

arokem Mar 13, 2017

Member

Oh yes - just what I needed!

Also adding a test for the color-map for MPL < 2!

@coveralls

This comment has been minimized.

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.007%) to 88.429% when pulling b5127f4 on arokem:deprecate-accent into 32ed56b on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Mar 13, 2017

Coverage Status

Coverage increased (+0.007%) to 88.429% when pulling b5127f4 on arokem:deprecate-accent into 32ed56b on nipy:master.

if have_matplotlib and mpl_version < "2":
names.append('Accent')
for name in names:

This comment has been minimized.

@matthew-brett

matthew-brett Mar 13, 2017

Member

Won't we need to wrap the mpl >= 2 / accent test in a catch_warnings block?

This comment has been minimized.

@arokem

arokem Mar 13, 2017

Member

Well, the warning is raised regardless of if you are using MPL > 2 or not, because we are deprecating this cmap for all users. But now I also implemented a test that checks if the warning is raised in the loop below as well.

# dipy's colormaps are close to matplotlibs colormaps, but not
# perfect:
npt.assert_array_almost_equal(rgba1, rgba2, 1)
if name == "Accent":

This comment has been minimized.

@matthew-brett

matthew-brett Mar 14, 2017

Member

How about:

npt.assert_(len(w) == (1 if name == 'Accent' else 0))

This comment has been minimized.

@arokem

arokem Mar 14, 2017

Member

Sure. That's neat.

@matthew-brett matthew-brett merged commit 26bfae6 into nipy:master Mar 14, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 14, 2017

Great - thanks a lot.

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1193 from arokem/deprecate-accent
MRG: Deprecate the Accent colormap, in anticipation of changes in MPL 2.0


Addresses nipy#1188
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment