Deprecate "hold" kwarg and machinery. by efiring · Pull Request #7516 · matplotlib/matplotlib

efiring · 2016-11-26T02:00:42Z

This follows #7515 and addresses #3070.

anntzer · 2016-11-26T04:50:23Z

lib/matplotlib/axes/_base.py

+        """return the HOLD status of the axes
+
+        The `hold` mechanism is deprecated and will be removed in
+        v3.0.


Can we not commit to a specific version here? Or at least make it one of 2.2, 2.3 or 2.4, which should be far away enough... Same comments below.

(mpl 3.0 sounds a bit like python 4 to me)

That is the correct version if we wish to follow SemVer.

... which we haven't been doing so far?

It was not fully concluded in #3070.

I would just say "in a future feature release" (as opposed to "bugfix release") in that case.

I thought this would generate some controversy... I would like to see "hold" go away ASAP, and I don't have strong opinions about version numbering. It seemed like the discussion in #3070 was leaning in the direction of going wild with major version numbers--why not, Firefox is up to 50.0! Seriously, regardless of how the numbering controversy is resolved, we need to find a way to get releases out more frequently.

We will at least have a 3.0 when we drop python2 support (so 2018).

We should be following SemVer, and we certainly are with 2.0. Therefore, when we remove hold it will be ++major_version. Is there any reason this wouldn't make 3.0?

NelleV · 2016-11-26T05:24:23Z

lib/matplotlib/axes/_axes.py

        y : sequence of scalars of length n

-        hold : boolean, optional, default: True
+        hold : boolean, optional, *deprecated*, default: True


The "correct" way to document deprecation with numpydoc is through the deprecation notes section, but I don't think this matters much.

This puts it out front, so I will consider it at least good enough for now.

NelleV

Overall it looks good, but I think you missed one deprecation warning.

NelleV · 2016-11-26T05:27:00Z

lib/matplotlib/pyplot.py

@@ -2486,24 +2499,26 @@ def getname_val(identifier):
 def _autogen_docstring(base):


Doesn't that function become obselete? The only thing it did is append the kwarg information on hold.

It is also handling the dedenting of the docstring from the Axes method, and adding a couple linefeeds. That is why I left it in.

NelleV · 2016-11-26T05:27:43Z

lib/matplotlib/pyplot.py

 # return an image or a line.
 @_autogen_docstring(Axes.spy)
-def spy(Z, precision=0, marker=None, markersize=None, aspect='equal', hold=None, **kwargs):
+def spy(Z, precision=0, marker=None, markersize=None, aspect='equal', **kwargs):


I think hold needs to be explicitly deprecated here as well.

Good catch. A fix is on the way.

This is the only place that hold in removed a positional argument. ~~I am mildly 👎 on breaking that.~~

I lost track of what file I was in (thought that was in _axes.py not pyplot.py. Still not enthusiastic about this change, at +0

spy is an obscure and peculiar function; 'hold' was the 6th argument, with only the first being in the signature as a purely positional argument. Although technically 'hold' could be used as a positional argument here in someone's code, I think the risk of serious damage from this change is minuscule, and worth taking for the sake of simplicity and consistency: keeping "hold" out of all function signatures.

NelleV · 2016-11-26T05:28:25Z

lib/matplotlib/pyplot.py

-        ax.hold(hold)
+        ax._hold = hold
+        from matplotlib.cbook import mplDeprecation
+        warnings.warn("The 'hold' kwarg is deprecated since 2.0.",


To be very nitpicky, the hold argument here is not a kwarg. (OK, no one cares…)

Hold is a keyword argument; are you asking me to spell that out, as opposed to using the common abbreviation? I don't mind doing that, I just want to be clear about what the nit is.

I don't think you should bother with that comment :)

I spelled it out.

NelleV · 2016-11-26T05:32:33Z

lib/matplotlib/rcsetup.py

        raise ValueError('Could not convert "%s" to boolean' % b)


+def deprecate_axes_hold(value):


Can you make this function private? It's unlikely that anyone is every going to use it, but who knows…

I could, but I was just following the pattern in rcsetup.py, which has several special-purpose functions like this. I think that it is OK to credit other programmers with enough common sense to realize that a function like this will go away when it is no longer needed. We might do more harm than good by trying to be too strict about leading underscores.

As long as it is very clear to all matplotlib developer that this function will disappear along with hold, that's OK with me, thought I'd rather have it private. Explicit is always better than implicit.

efiring · 2016-11-26T05:59:22Z

On 2016/11/25 7:48 PM, Nelle Varoquaux wrote: Explicit is always better than implicit.

Come on, that's no fun! Keep people on their toes, keep them guessing!

NelleV · 2016-11-26T21:15:03Z

Once you think this is ready to be merged, can you ping us on it or add the [MRG] tag to the PR?

efiring · 2016-11-27T02:45:47Z

I think this is in reasonable shape now. The messages specifying 3.0 as the removal point are subject to debate; but if the decision is to phrase them differently, that change could be a separate tiny PR.

To make the deprecation strategy explicit:

This is stage one: if anyone tries to set a "hold" value, it will trigger a warning; otherwise, there should be no change in the behavior.
The next stage could be to change all the warnings to exceptions, enabling the removal of part, but not all, of the "hold" system. I view this as an optional stage, not an essential one. I would prefer to skip it.
The final stage is gleefully deleting every last vestige of the "hold" machinery.

Basemap needs to be treated in sync with mpl, but it can be done in a simpler fashion. I will submit a PR for that now.

QuLogic · 2016-11-27T02:48:12Z

doc/api/api_changes.rst

+The behavior will remain consistent with the default ``hold=True``
+state that has long been in place.  Instead of using a function
+or keyword argument (``hold=False``) to change that behavior,
+explicitly clear the axes or figure as needed prior to subsequent


Maybe link these two things?

Sorry, I don't understand what you are suggesting.

A cross-reference to the relevant functions for clearing the Axes/Figure.

QuLogic · 2016-11-27T02:59:11Z

boilerplate.py

+        %(ax)s._hold = hold
+        from matplotlib.cbook import mplDeprecation
+        warnings.warn("The 'hold' keyword argument is deprecated since 2.0.",
+                      mplDeprecation)


Should we use stacklevel=2, so that the warning references the caller instead of pyplot.py?

I tried that just now, but it gives puzzling result when using pyplot commands from the command line, or from a function defined on the command line. I think we are better off leaving the default in place.

Hmm, really?

>>> import numpy as np >>> import matplotlib.pyplot as plt >>> plt.plot(np.arange(10), hold=False) /home/elliott/code/matplotlib/lib/matplotlib/pyplot.py:3315: MatplotlibDeprecationWarning: The 'hold' keyword argument is deprecated since 2.0. mplDeprecation) [<matplotlib.lines.Line2D object at 0x7f3722d1f320>]

vs.

>>> import numpy as np >>> import matplotlib.pyplot as plt >>> plt.plot(np.arange(10), hold=False) __main__:1: MatplotlibDeprecationWarning: The 'hold' keyword argument is deprecated since 2.0. [<matplotlib.lines.Line2D object at 0x7f83598b5c50>]

looks correct to me.

This is with ipython and stacklevel=2:

In [1]: plot([1,2,3], hold=True) /Users/efiring/anaconda/envs/test/bin/ipython:1: MatplotlibDeprecationWarning: The 'hold' keyword argument is deprecated since 2.0. #!/bin/bash /Users/efiring/anaconda/envs/test/bin/python.app [<matplotlib.lines.Line2D at 0x1151f1f98>] In [2]: def make_warning(): ...: plot([1,2,3], hold=True) ...: In [3]: make_warning() /Users/efiring/anaconda/envs/test/bin/ipython:2: MatplotlibDeprecationWarning: The 'hold' keyword argument is deprecated since 2.0. if __name__ == '__main__':

and with the default:

In [1]: plot([1,2,3], hold=True) /Users/efiring/work/programs/py/mpl/matplotlib/lib/matplotlib/pyplot.py:3315: MatplotlibDeprecationWarning: The 'hold' keyword argument is deprecated since 2.0. mplDeprecation) [<matplotlib.lines.Line2D at 0x117dce0b8>] In [2]: def make_warning(): plot([1,2,3], hold=True) ...: In [3]: make_warning() /Users/efiring/work/programs/py/mpl/matplotlib/lib/matplotlib/pyplot.py:3315: MatplotlibDeprecationWarning: The 'hold' keyword argument is deprecated since 2.0. mplDeprecation)

For this use, the default looks better to me. What would really be needed for someone to debug a hold kwarg buried in a library is a full stack trace, but we are not going to put that in.

NelleV · 2016-11-27T19:10:23Z

Thanks for taking hold down, @efiring ! The patch looks good to me.

tacaswell · 2016-11-27T19:31:11Z

👍 merged and did the merge up to master

efiring · 2016-11-27T19:35:18Z

On 2016/11/27 9:31 AM, Thomas A Caswell wrote: merged and did the merge up to master

Thank you!

efiring added this to the 2.0 (style change major release) milestone Nov 26, 2016

efiring force-pushed the kill_hold_stage2 branch 2 times, most recently from 65a10b9 to 5e955a2 Compare November 26, 2016 03:23

QuLogic added the API: changes label Nov 26, 2016

Deprecate "hold" kwarg and machinery.

7c9d4a6

efiring force-pushed the kill_hold_stage2 branch from 5e955a2 to 7c9d4a6 Compare November 26, 2016 04:43

anntzer reviewed Nov 26, 2016

View reviewed changes

NelleV reviewed Nov 26, 2016

View reviewed changes

Add missing deprecation; spell out "keyword argument"

f08a0fb

efiring added 2 commits November 26, 2016 08:45

Remove axes.hold from classic.mplstyle

785a516

DOC: add entry to api_changes; improve some docstrings

dce381e

efiring added 3 commits November 26, 2016 12:43

Remove calls to hold(True) from test_axes.py

403ab99

Use None as a sentinel value for unmodified axes.hold in rcParams.

219ce30

Convert two hold() calls in axes_grid1 to attribute assignments

0cc0f57

efiring changed the title ~~Deprecate "hold" kwarg and machinery.~~ [MRG] Deprecate "hold" kwarg and machinery. Nov 27, 2016

efiring mentioned this pull request Nov 27, 2016

Convert hold-related functions to attribute assignments. matplotlib/basemap#328

Merged

QuLogic reviewed Nov 27, 2016

View reviewed changes

NelleV approved these changes Nov 27, 2016

View reviewed changes

NelleV changed the title ~~[MRG] Deprecate "hold" kwarg and machinery.~~ [MRG+1] Deprecate "hold" kwarg and machinery. Nov 27, 2016

tacaswell merged commit 8251436 into matplotlib:v2.x Nov 27, 2016

efiring deleted the kill_hold_stage2 branch November 27, 2016 19:36

efiring mentioned this pull request Nov 27, 2016

remove hold logic from library #3070

Closed

QuLogic changed the title ~~[MRG+1] Deprecate "hold" kwarg and machinery.~~ Deprecate "hold" kwarg and machinery. Nov 28, 2016

		@@ -2486,24 +2499,26 @@ def getname_val(identifier):
		def _autogen_docstring(base):

		raise ValueError('Could not convert "%s" to boolean' % b)


		def deprecate_axes_hold(value):

Uh oh!

Conversation

efiring commented Nov 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell Nov 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiring commented Nov 26, 2016 via email

Uh oh!

NelleV commented Nov 26, 2016

Uh oh!

efiring commented Nov 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV commented Nov 27, 2016

Uh oh!

tacaswell commented Nov 27, 2016

tacaswell Nov 27, 2016 •

edited

Loading