removed deprecated methods from the axes module. #1568

Merged
merged 4 commits into from Jan 16, 2013

Projects

None yet

5 participants

@NelleV
Collaborator
NelleV commented Dec 7, 2012

This PR not only removes deprecated methods, but also add deprecated warnings for methods that were said as being deprecated, but no warning had ever been added.

All the tests pass.

Thanks,
N

@NelleV
Collaborator
NelleV commented Dec 7, 2012

Unlike what I said, travis-ci tells me the tests don't pass... i'll have a closer look.

@efiring
Member
efiring commented Dec 7, 2012

I think that you will also want to use the new MatplotlibDeprecationWarning; I'm not sure whether it has been merged yet, but it is the way to go.

@NelleV
Collaborator
NelleV commented Dec 7, 2012

It has not been merged yet. I'll update the PR once it is.

@efiring efiring and 1 other commented on an outdated diff Dec 7, 2012
lib/matplotlib/axes.py
@@ -6137,6 +6076,9 @@ def scatter(self, x, y, s=20, c='b', marker='o', cmap=None, norm=None,
else:
colors = mcolors.colorConverter.to_rgba_array(c, alpha)
+ # FIXME: faceted is deprecated but no warning is set when faceted is
+ # manually set to True.
+ # Moreover, is edgecolors 'none' and None different ?
@efiring
efiring Dec 7, 2012 Matplotlib Developers member

Yes, None means use the rcParams['patch.edgecolor'], but "none" means "don't color the edges at all".

@NelleV
NelleV Dec 7, 2012 collaborator

OK, good to now (I could have looked it up).

One way to fix the deprecation warning problem I mention just above would be to change the default value of faceted to spot when it is manually set to True, and raise the warning.
I'm not sure it is the best way to go, but that's the only thing my (very tired) brain is coming up with right now...

@efiring
efiring Dec 7, 2012 Matplotlib Developers member

@nelleV I think the thing to do here, after you have had a good rest, is to remove faceted from the listed kwargs. Then

faceted = kwargs.pop('faceted', None)
if faceted is not None:
    # raise the deprecation warning...
    # and set edgecolors as is already being done based on the value of faceted...
@NelleV
NelleV Dec 7, 2012 collaborator

It's done !

@efiring efiring and 1 other commented on an outdated diff Dec 7, 2012
lib/matplotlib/axes.py
@@ -6068,13 +6006,17 @@ def scatter(self, x, y, s=20, c='b', marker='o', cmap=None, norm=None,
else:
colors = mcolors.colorConverter.to_rgba_array(c, alpha)
- if faceted:
- edgecolors = None
- else:
- edgecolors = 'none'
- warnings.warn(
- '''replace "faceted=False" with "edgecolors='none'"''',
- DeprecationWarning) # 2008/04/18
+ # FIXME: faceted is deprecated but no warning is set when faceted is
@efiring
efiring Dec 7, 2012 Matplotlib Developers member

No need for this comment now.

@NelleV
NelleV Dec 7, 2012 collaborator

Indeed - I deleted it.

@dmcdougall dmcdougall commented on the diff Dec 8, 2012
lib/matplotlib/axes.py
@@ -1067,11 +1047,16 @@ def set_aspect(self, aspect, adjustable=None, anchor=None):
etc.
===== =====================
+ .. deprecated:: 1.2
@dmcdougall
dmcdougall Dec 8, 2012 Matplotlib Developers member

If we're deprecating this in 1.2, should this PR be targeting the v1.2.x instead?

@NelleV
NelleV Dec 8, 2012 collaborator

It was deprecated before: the only "problem" is that it was mention in the documentation, but no deprecation warning was raised. I'm not sure how to use this documentation tag: should I put the version were it was tag as being deprecated, or the version were we target the removal of the option ?

@dmcdougall
dmcdougall Dec 8, 2012 Matplotlib Developers member

Aha... I see. Ok. No, I think you have used the tag correctly.

@NelleV
Collaborator
NelleV commented Dec 8, 2012

I'm now using the new matplotlib deprecation warning.

@ivanov ivanov commented on an outdated diff Dec 14, 2012
lib/matplotlib/axes.py
@@ -6068,13 +6008,15 @@ def scatter(self, x, y, s=20, c='b', marker='o', cmap=None, norm=None,
else:
colors = mcolors.colorConverter.to_rgba_array(c, alpha)
- if faceted:
- edgecolors = None
- else:
- edgecolors = 'none'
- warnings.warn(
- '''replace "faceted=False" with "edgecolors='none'"''',
- DeprecationWarning) # 2008/04/18
+ faceted = kwargs.pop('faceted', None)
+ if faceted is not None:
+ raise MDeprecation("The faceted option is deprecated. "
+ "Please use edgecolor instead. Will "
+ "be remove in 1.4")
@ivanov
ivanov Dec 14, 2012 Matplotlib Developers member

By raising here, this functionality is eliminated, there's no way for the code path to continue. So either keeps this as a warning, or raise with just "The faceted option is deprecated. Please use edgecolor instead."

@ivanov ivanov commented on an outdated diff Dec 14, 2012
lib/matplotlib/axes.py
@@ -7285,6 +7227,9 @@ def pcolor(self, *args, **kwargs):
cmap = kwargs.pop('cmap', None)
vmin = kwargs.pop('vmin', None)
vmax = kwargs.pop('vmax', None)
+ if shading in kwargs:
+ raise MDeprecation("Use edgecolors instead of shading. "
+ "Will be removed in 1.4")
@ivanov
ivanov Dec 14, 2012 Matplotlib Developers member

again, this will just error out, use warnings.warn("message", mDeprecation) or warnings.warn(mDeprecation('message')

@ivanov ivanov commented on an outdated diff Dec 14, 2012
lib/matplotlib/axes.py
def get_frame(self):
"""Return the axes Rectangle frame"""
- warnings.warn('use ax.patch instead', DeprecationWarning)
+ raise MDeprecation('use ax.patch instead')
@ivanov
ivanov Dec 14, 2012 Matplotlib Developers member

this, too, is a change in functionality, but seems like the right one (raising instead of just issuing a warning)

@ivanov
Member
ivanov commented Dec 14, 2012

I left a few comments inline, but wanted to add that since this now uses MatplotlibDeprecationWarning which currently lives in #1597, this PR should be merged after that one.

@NelleV
Collaborator
NelleV commented Dec 30, 2012

I've updated this patch with the new interface.

@NelleV
Collaborator
NelleV commented Jan 7, 2013

I think this is ready to be merged.

@WeatherGod WeatherGod and 1 other commented on an outdated diff Jan 7, 2013
lib/matplotlib/axes.py
@@ -7307,6 +7251,9 @@ def pcolor(self, *args, **kwargs):
cmap = kwargs.pop('cmap', None)
vmin = kwargs.pop('vmin', None)
vmax = kwargs.pop('vmax', None)
+ if 'shading' in kwargs:
+ raise DeprecationWarning("Use edgecolors instead of shading. "
@WeatherGod
WeatherGod Jan 7, 2013 Matplotlib Developers member

Shouldn't this be a mplDeprecation?

@NelleV
NelleV Jan 7, 2013 collaborator

Nice catch. I've fixed it.

@NelleV
Collaborator
NelleV commented Jan 16, 2013

@dmcdougall Maybe you can have a look at this PR ? It should be quite staightforward and ready for merge.

@dmcdougall
Member

@NelleV Code style looks good, but Travis is legitimately failing. I will run the tests locally and try to help you through fixing the errors.

@dmcdougall dmcdougall was assigned Jan 16, 2013
@dmcdougall dmcdougall commented on the diff Jan 16, 2013
lib/matplotlib/axes.py
- if faceted:
- edgecolors = None
- else:
- edgecolors = 'none'
- warnings.warn(
- '''replace "faceted=False" with "edgecolors='none'"''',
- mplDeprecation) # 2008/04/18
+ faceted = kwargs.pop('faceted', None)
+ if faceted is not None:
+ warnings.warn("The faceted option is deprecated. "
+ "Please use edgecolor instead. Will "
+ "be remove in 1.4", mplDeprecation)
+ if faceted:
+ edgecolors = None
+ else:
+ edgecolors = 'none'
@dmcdougall
dmcdougall Jan 16, 2013 Matplotlib Developers member

In the scatter test, this code never gets executed and edgecolors doesn't get set.

@NelleV
NelleV Jan 16, 2013 collaborator

I've fixed the problem. I think there was a bug before, as edgecolors was always set from the argument faceted, instead of being fetched from the kwargs.

I also think we need to consider moving it from kwargs to a named argument. What do you think?

@dmcdougall
dmcdougall Jan 16, 2013 Matplotlib Developers member

I think that, to be consistent with the other pyplot functions, we should leave it as a kwarg and perhaps formulate an MEP to overhaul the kwargs if there is a consensus for that.

@dmcdougall
Member

Ok, since that edgecolors fix, the tests pass on 2.7 and 3.2 and we get dud failures for 2.6 and 3.1. I'm going to merge this.

Thanks @NelleV.

@dmcdougall dmcdougall merged commit 8943757 into matplotlib:master Jan 16, 2013

1 check failed

Details default The Travis build failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment