Convert **kwargs to named arguments for a clearer API #11137

Zac-HD · 2018-04-27T16:00:04Z

This pull request converts *args, **kwargs to use named arguments in a variety of places, as suggested in #9912.

There are no changes to the API; this is just using a Python-3-only syntax to make the existing API more explicit. There is a small amount of related refactoring, where I clarified a method or removed a unused variable.

CC @timhoffm, who opened the issue 😄

anntzer · 2018-04-27T21:41:09Z

lib/matplotlib/axes/_axes.py

@@ -1433,7 +1433,8 @@ def plot_date(self, x, y, fmt='o', tz=None, xdate=True, ydate=False,

    # @_preprocess_data() # let 'plot' do the unpacking..
    @docstring.dedent_interpd
-    def loglog(self, *args, **kwargs):
+    def loglog(self, *args, basex=10, basey=10, subsx=None, subsy=None,
+               nonposx='mask', nonposy='mask', **kwargs):


I think it looks slightly better (though semantically identical) to have e.g.

def loglog(self, *args, basex=..., subsx=..., ... basey=..., ... **kwargs):

(but ymmv)

anntzer · 2018-04-27T21:45:22Z

lib/matplotlib/pyplot.py

@@ -1552,7 +1552,7 @@ def yscale(scale, **kwargs):
    gca().set_yscale(scale, **kwargs)


-def xticks(*args, **kwargs):
+def xticks(locs=None, labels=None, **kwargs):


What are the names of these arguments in other similar parts of the API? (i.e. are "locs" and "labels" what we want to call these?)
Same for yticks.

I couldn't see a directly analogous part of the API, but these names are taken directly from the docstring where it describes the call signatures.

Actually the argument for Axes.set_xticks and Axis.set_ticks is named ticks, so let's stay consistent and change the docstring accordingly (remember that argument names effectively become part of the public API).
The argument for Axes.set_xticklabels is named labels but the one for Axis.set_ticklabels is ticklabels, which is unfortunate but I think labels is fine.

anntzer · 2018-04-27T21:48:09Z

lib/mpl_toolkits/mplot3d/axes3d.py

@@ -1357,11 +1360,6 @@ def ticklabel_format(
            sb = True
        elif style in ['plain', 'comma']:
            sb = False
-            if style == 'plain':


just drop support for 'comma' style (deprecation, yada yada)? that doesn't exist for regular axes either.

This doesn't actually drop support for anything - the cb variable was never used after being assigned!

Zac-HD · 2018-04-28T02:34:57Z

Turns out the default behaviour for ax.semilogy was incorrectly documented - non-positive coordinates are clipped to just above zero, rather than masked. This was probably copied from semilogx (which does mask), and the docs not updated when someone changed the behaviour.

anntzer · 2018-04-28T02:51:47Z

I would argue that that's a behavioral bug (the intent is certainly that both behave in the same way...)

Zac-HD · 2018-04-28T04:06:57Z

I would argue that that's a behavioural bug (the intent is certainly that both behave in the same way...)

I actually agree, but fixing bugs is out of scope for this PR - it's large enough already! I've therefore just removed the log-related changes, with an aspiration to do them later.

timhoffm · 2018-04-28T09:28:24Z

We should somehow mention the additional parameters in the docstring. Suggestion: Add them to the existing params:

left, xmin : scalar, optional
    The left xlim (default: None, which leaves the left limit
    unchanged). *xmin* is supported as an alteranative name for *left*.

alternatively use a separate entry:

xmin, xmax : scalar, optional
    *xmin*, *xmax* can be used as an alternative to *left*, *right*.

What is our recommendation? Are they equally or do we recommend one and the other is just there for backward compatibility. If so, that should be documented as well.

dstansby · 2018-04-28T11:32:57Z

lib/matplotlib/axes/_base.py

@@ -3016,7 +3016,8 @@ def _validate_converted_limits(self, limit, convert):
                raise ValueError("Axis limits cannot be NaN or Inf")
            return converted_limit

-    def set_xlim(self, left=None, right=None, emit=True, auto=False, **kw):
+    def set_xlim(self, left=None, right=None, emit=True, auto=False,
+                 *, xmin=None, xmax=None):


Would it be worth deprecating xmin and xmax or left or right? Seems a bit silly having two ways of specifying what is really the same argument.

+1 that two ways are a bit silly. Not sure if we have to go as far as deprecating (but wouldn‘t object either. Maybe go slow with a PendingDeprecation). Anyway, there should be one recommended way and the other only is only a backward compatible fallback.

The question is which to recommend:

xmin, xmax is more generic and easy to transfer, e.g. ymin rmin

left, right is more semantic

To me +0.5 for keeping xmin, xmax

Actually, left/right is clearly better IMO: one can pass left > right for inverted axes, which sounds less silly that xmin > xmax...

This code path is due to an API change from @efiring in 2010 (9ca5db0) and this has not been documented in the docstring. I am inclined to deprecate these unless we find that they are widely used in the wild.

I am also 👍 on pushing the deprecation work to a different PR.

dstansby · 2018-04-28T11:34:08Z

lib/matplotlib/axes/_base.py

        if right is None and iterable(left):
            left, right = left
+        if xmin is not None:
+            if left is not None:
+                raise ValueError('Cannot use argument xmin=%r; would '


This technically breaks backwards compatibility I think - previously passing xmin and left would silently use the value in xmin, whereas now it raises an error.

Yes. Should just issue a warning. But what to be done in this context depends on the decision on deprecation.

Zac-HD · 2018-04-28T14:28:11Z

OK, update on the xmin/xmax situation:

Personally I'd deprecate them, but that's out of scope for this PR, so I'll treat them as an ongoing part of the API (at least for now). IMO xmin is nicer than left, but not by enough to beat the current-posarg premium.
Fair point on breaking compatibility with a new error. I've changed that to a warning when a supplied argument is being overridden, to avoid silently throwing away user input.
I've documented them in a separate entry in the parameters docs, including the overriding behaviour.

Zac-HD · 2018-04-28T15:07:27Z

🎉 Everything passes! If there are no additional review items, I'd like to get this merged soon so I can open the next PR in the sequence 😄

lib/matplotlib/axes/_base.py

anntzer

per https://github.com/matplotlib/matplotlib/pull/11137/files#r184865106
anyone can dismiss this once the comment is handled.

tacaswell · 2018-04-28T23:37:16Z

lib/matplotlib/axes/_base.py

@@ -2744,8 +2744,6 @@ def ticklabel_format(self, *, axis='both', style='', scilimits=None,
            sb = True
        elif style == 'plain':
            sb = False
-        elif style == 'comma':
-            raise NotImplementedError("comma style remains to be added")


Can you add an API changes note for this? I don't think we need to anything about it code-wise, but should document that an exception type has changed.

tacaswell · 2018-04-28T23:39:28Z

lib/mpl_toolkits/mplot3d/axes3d.py

@@ -558,9 +555,6 @@ def autoscale_view(self, tight=None, scalex=True, scaley=True,
            self.set_xbound(x0, x1)

        if scaley and self._autoscaleYon:
-            yshared = self._shared_y_axes.get_siblings(self)


Are we sure that these have no side-effects?

I've just gone back and traced through carefully - it does have the effect of self._shared_y_axes.clean(), so I've replaced the block with that. Not sure if this is actually needed, but better safe than sorry!

tacaswell · 2018-04-28T23:46:46Z

lib/matplotlib/axes/_base.py

@@ -3016,7 +3016,8 @@ def _validate_converted_limits(self, limit, convert):
                raise ValueError("Axis limits cannot be NaN or Inf")
            return converted_limit

-    def set_xlim(self, left=None, right=None, emit=True, auto=False, **kw):
+    def set_xlim(self, left=None, right=None, emit=True, auto=False,
+                 *, xmin=None, xmax=None):


This code path is due to an API change from @efiring in 2010 (9ca5db0) and this has not been documented in the docstring. I am inclined to deprecate these unless we find that they are widely used in the wild.

I am also 👍 on pushing the deprecation work to a different PR.

lib/matplotlib/axes/_base.py

tacaswell · 2018-04-28T23:49:34Z

Thanks @Zac-HD ! I am very happy to see this sort of work going in. We are probably going to be picking about API changes and while that may be frustrating / slow with your developer hat on, hopefully you appreciate it with your user hat on!

Zac-HD · 2018-04-29T00:42:07Z

I am very happy to see this sort of work going in. We are probably going to be picking about API changes and while that may be frustrating / slow with your developer hat on, hopefully you appreciate it with your user hat on!

Also happy and much more appreciative than frustrated - the quick reviews make it a pleasure to contribute.

Zac-HD · 2018-04-29T02:07:24Z

I've documented the style='comma' change of exception type
The loc argument has been renamed ticks as requested by @anntzer
I've gone ahead and implemented the exception and deprecation for the *min arguments. It's kinda tangential to the main purpose of this PR, but with a new exception and new docs I'd rather jump directly to the end goal than risk leaving it half-finished.

If the build passes, I think I'm done (again 😉).

timhoffm

Sorry, still some comments. But you're almost there. 😄

lib/matplotlib/axes/_base.py

lib/matplotlib/pyplot.py

Zac-HD · 2018-04-29T10:18:29Z

We now have enough things ruled out of scope that I need a list to keep track! After this is merged, I'll open an issue for each of them:

Axes.semilogy has different default behaviour for out-of-bounds coords than Axes.semilogx - the former clips them to 0+eps, while in the x-dimension they're masked. The latter is the correct behaviour. IIRC Axes.loglog is also affected. (done, Make Axes.semilogx and semilogy consistent in handling negative coords #11193)
Axes.set_x/yticks uses the name ticks for the array of tick locations. It's proposed here (comment) to use locations instead, and we'll want to change set_x/yticks over a deprecation cycle to be consistent.
I have a branch with many more **kwargs-to-named-args conversion, for PRs after this is merged.

jklymak

This looks good to me. Thanks!

I think your point was addressed.

jklymak · 2018-05-01T03:52:36Z

... though I'm a little concerned that the codecov scores dropped...

Zac-HD · 2018-05-01T06:12:17Z

... though I'm a little concerned that the codecov scores dropped...

This is one of the problems with percent coverage as a metric! It's not that there are more lines without coverage - instead there are fewer lines with coverage, because e.g. all the kwargs.pop lines were trivially covered. Thus, the percentage has gone down even though the tests are no weaker and the library is harder to misuse.

(The only generally useful metric for coverage is "what behaviour has not been executed by my test suite?", and percentage measures can be usefully broken down into "100%" and "less than 100%" but not much further. /rant over, sorry.)

QuLogic · 2018-05-01T06:16:07Z

That's not entirely true; there are some new lines without coverage, e.g., all the checks for xmin vs. left, etc.

Zac-HD · 2018-05-01T06:45:18Z

The deprecation and error are new, true, and I could write trivial tests that they're emitted if someone thinks that's really necessary.

I do think most of this is because dead-but-covered code was removed though.

timhoffm

This is fine by me. The new uncovered code lines are basically the code for handling the deprecated xmin/xmax attributes. I don't think it's neccessary to add tests for these. These code segments are simple enough that I trust them without a test and they will be gone in a while.

I'll still leave this open for a short time in case somebody insists on tests.

jklymak

Blocking just on @efiring review request from @tacaswell. Feel free to clear this once he has looked at it, I don't have any problems w/ this.

jklymak · 2018-05-01T14:12:32Z

The deprecation and error are new, true, and I could write trivial tests that they're emitted if someone thinks that's really necessary.

I don't think its necessary. Just hadn't thought it through enough to know if it was going to impact other PRs after this is merged.

jkseppan

Looks good to me!

Zac-HD · 2018-05-02T08:14:48Z

Rebased on master to remove merge conflict from #11145; no other changes.

Zac-HD · 2018-05-03T04:05:24Z

Ping for @efiring - we're just waiting on your approval to merge (or comments to fix), which will unblock another sequence from me 😄

Zac-HD · 2018-05-08T00:09:53Z

Ping @timhoffm @jklymak @tacaswell - this has been waiting for a review from @efiring for a full week now, and having it sit here unmerged is blocking some other refactoring I'd like to do. I'm also getting pretty sick of rebasing it on master to fix merge conflicts!

Could someone contact him and ask for this to be prioritised, or just bite the bullet and merge it with only three approvals?

(@jklymak's change request is really "approved, but wait for Eric")

stale

timhoffm · 2018-05-08T06:11:26Z

@efiring Should only need to comment on the deprecation of the xmin, xmax code path. This was decided to be handled in a separate PR anyway. The present PR only makes the existing signature explicit. I'm going to merge as an improvement.

efiring · 2018-05-08T06:33:37Z

Sorry to have been AWOL on this. I think I need to reconfigure my email notifications so that I automatically get only the ones that mention me or that I have weighed in on, instead of getting one for absolutely everything.

Zac-HD · 2018-05-08T07:20:35Z

No worries Eric! I disabled email updates entirely a while ago, and personally I've found sticking to the web interface for notifications has helped a lot.

And thanks to everyone in the thread for your feedback - the result has been a much better patch 😄

These were originally documented and deprecated in matplotlib#11137 however in later discussion we learned: - the 'left, right' / 'top, bottom' names don't make sense in the context of non-rectangular plots - we have at least one frustrated user The original names were `xmin, xmax` but were changed to `left, right` in 9ca5db0 (pre 1.0) and the kwarg popping was added for back-compatibility.

Zac-HD changed the title ~~Issue 9912 cleanup~~ Convert **kwargs to named arguments for a clearer API Apr 27, 2018

anntzer reviewed Apr 27, 2018

View reviewed changes

Zac-HD force-pushed the issue-9912-cleanup branch from 944a0e3 to 21e9e65 Compare April 28, 2018 02:32

Zac-HD force-pushed the issue-9912-cleanup branch from 21e9e65 to 623b98d Compare April 28, 2018 05:18

dstansby added this to the v3.0 milestone Apr 28, 2018

dstansby reviewed Apr 28, 2018

View reviewed changes

Zac-HD force-pushed the issue-9912-cleanup branch from 623b98d to 1ea943f Compare April 28, 2018 14:24

dstansby reviewed Apr 28, 2018

View reviewed changes

lib/matplotlib/axes/_base.py Outdated Show resolved Hide resolved

anntzer previously requested changes Apr 28, 2018

View reviewed changes

tacaswell reviewed Apr 28, 2018

View reviewed changes

Zac-HD force-pushed the issue-9912-cleanup branch from d0e746b to 4dc70e5 Compare April 29, 2018 01:50

Zac-HD force-pushed the issue-9912-cleanup branch from 4dc70e5 to d43b0fe Compare April 29, 2018 02:40

timhoffm reviewed Apr 29, 2018

View reviewed changes

lib/matplotlib/axes/_base.py Outdated Show resolved Hide resolved

lib/matplotlib/pyplot.py Show resolved Hide resolved

tacaswell requested a review from efiring April 29, 2018 20:13

This was referenced Apr 30, 2018

Use kwonlyargs instead of popping from kwargs #11145

Merged

Explicit args and refactor Axes.margins #11146

Merged

jklymak approved these changes May 1, 2018

View reviewed changes

timhoffm approved these changes May 1, 2018

View reviewed changes

jklymak previously requested changes May 1, 2018

View reviewed changes

jkseppan approved these changes May 1, 2018

View reviewed changes

Zac-HD force-pushed the issue-9912-cleanup branch from d59ed4d to 641aa50 Compare May 2, 2018 08:07

Zac-HD added 4 commits May 8, 2018 10:04

Use named args for xticks, yticks in pyplot

6be9c9f

Clarify and check alternative names for axis limits

09991e6

Minor refactor for clarity

5720387

Eliminate unused variables in axes3d

c408392

Zac-HD force-pushed the issue-9912-cleanup branch from 641aa50 to c408392 Compare May 8, 2018 00:06

Zac-HD mentioned this pull request May 8, 2018

Make Axes.semilogx and semilogy consistent in handling negative coords #11193

Closed

timhoffm merged commit 4a12a60 into matplotlib:master May 8, 2018

Zac-HD deleted the issue-9912-cleanup branch May 8, 2018 07:18

timhoffm mentioned this pull request May 23, 2018

Lim parameter naming #11293

Merged

Zac-HD mentioned this pull request Jun 27, 2018

Raise TypeError on unsupported kwargs of spy() #11367

Merged

timhoffm mentioned this pull request Jul 23, 2018

Still naming inconsistency in API on axes limits #11761

Closed

tacaswell mentioned this pull request Feb 13, 2019

API: un-deprecate keyword only args to set_xlim, set_ylim #13425

Merged

5 tasks

Convert **kwargs to named arguments for a clearer API #11137

Convert **kwargs to named arguments for a clearer API #11137

Conversation

Zac-HD commented Apr 27, 2018

Choose a reason for hiding this comment

anntzer Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD Apr 28, 2018 • edited Loading

Choose a reason for hiding this comment

Zac-HD commented Apr 28, 2018

anntzer commented Apr 28, 2018

Zac-HD commented Apr 28, 2018 • edited Loading

timhoffm commented Apr 28, 2018 • edited Loading

Choose a reason for hiding this comment

timhoffm Apr 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD commented Apr 28, 2018 • edited Loading

Zac-HD commented Apr 28, 2018

anntzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Apr 28, 2018

Zac-HD commented Apr 29, 2018

Zac-HD commented Apr 29, 2018

timhoffm left a comment

Choose a reason for hiding this comment

Zac-HD commented Apr 29, 2018 • edited Loading

jklymak left a comment

Choose a reason for hiding this comment

jklymak commented May 1, 2018

Zac-HD commented May 1, 2018 • edited Loading

QuLogic commented May 1, 2018

Zac-HD commented May 1, 2018

timhoffm left a comment

Choose a reason for hiding this comment

jklymak left a comment

Choose a reason for hiding this comment

jklymak commented May 1, 2018

jkseppan left a comment

Choose a reason for hiding this comment

Zac-HD commented May 2, 2018

Zac-HD commented May 3, 2018

Zac-HD commented May 8, 2018 • edited Loading

timhoffm commented May 8, 2018

efiring commented May 8, 2018

Zac-HD commented May 8, 2018

anntzer Apr 27, 2018 •

edited

Loading

Zac-HD Apr 28, 2018 •

edited

Loading

Zac-HD commented Apr 28, 2018 •

edited

Loading

timhoffm commented Apr 28, 2018 •

edited

Loading

timhoffm Apr 28, 2018 •

edited

Loading

Zac-HD commented Apr 28, 2018 •

edited

Loading

Zac-HD commented Apr 29, 2018 •

edited

Loading

Zac-HD commented May 1, 2018 •

edited

Loading

Zac-HD commented May 8, 2018 •

edited

Loading