Change manual kwargs popping to kwonly arguments. by anntzer · Pull Request #10545 · matplotlib/matplotlib

anntzer · 2018-02-20T09:32:25Z

Only simple cases (no mutable defaults, no defaults depending on other
args) are handled so far.

PR Summary

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

timhoffm · 2018-02-20T12:49:49Z

lib/matplotlib/contour.py

-    def clabel(self, *args, **kwargs):
+    def clabel(self, *args,
+               fontsize=None, inline=True, inline_spacing=5, fmt='%1.3f',
+               colors=None, use_clabeltext=False, manual=False):


rightside_up missing

timhoffm · 2018-02-20T12:57:39Z

lib/matplotlib/backends/backend_pdf.py

-    def print_pdf(self, filename, **kwargs):
-        image_dpi = kwargs.get('dpi', 72)  # dpi to use for images
+    def print_pdf(self, filename, *,
+                  dpi=72,  # dpi to use for images


Maybe deprecate dpi and replace by image_dpi? During the deprecation dpi could be catched via kwargs.

nah, let's try to keep all the print_foo's with as much the same signature as possible

timhoffm · 2018-02-20T13:00:41Z

lib/matplotlib/backends/backend_pgf.py


-    def _print_pgf_to_fh(self, fh, *args, **kwargs):
-        if kwargs.get("dryrun", False):
+    def _print_pgf_to_fh(self, fh, *args,


*args is unused -> * only

see above re: keeping the same signature for all print_foo's. In any case this would mean fixing the call sites as well, so it'll be separate work.

timhoffm · 2018-02-20T13:12:01Z

lib/matplotlib/tests/test_figure.py

    class MyAxes(Axes):
-        def __init__(self, *args, **kwargs):
-            kwargs.pop('myclass', None)
+        def __init__(self, *args, myclass=None **kwargs):


missing comma

timhoffm · 2018-02-20T13:15:41Z

lib/mpl_toolkits/axes_grid1/parasite_axes.py

    return ax

-def host_subplot(*args, **kwargs):
+def host_subplot(*args, axes_class=None, **kwargs):


missing figure kwarg

timhoffm · 2018-02-20T13:17:36Z

lib/mpl_toolkits/axisartist/axes_grid.py

-        orientation=kwargs.pop("orientation", None)
-        if orientation is None:
-            raise ValueError("orientation must be specified")
+    def __init__(self, *args, orientation, **kwargs):


Not exactly the same: You can now explicitly pass orientation=None and it will not raise a ValueError.

orientation="foobar" was probably also invalid to start with, so there must be a downstream check that will also prune None out.

Maybe you're right. I don't follow the code path here. And I couldn't find out whose responsibility checking or orientation really is.

When doing

ImageGrid(plt.figure(), (0, 0, 1,1), (1, 2), cbar_mode='single', cbar_location='foobar')

it finally raises a KeyError on ax.axis[self.orientation].toggle(all=b) in CbarAxesBase._config_axes(). However that seems more by chance than an intended check.

Before that we have code executed like

if self.orientation in ["top", "bottom"]: orientation = "horizontal" else: orientation = "vertical"

which is locally coercing all invalid values to 'vertical'.

The whole thing seems a little brittle to me. But this is beyond this PR. By me, it's ok to not reject orientation=None in this case. Some code downstream will complain.

anntzer · 2018-02-20T16:07:26Z

thanks, comments handled

timhoffm

I don't understand the Travis build failure, but it looks more like a setup issue in the build rather than an error due to this PR.

anntzer · 2018-02-22T03:42:08Z

rebased

anntzer · 2018-03-15T19:14:46Z

rebased

jklymak · 2018-03-15T19:26:37Z

I'd need more motivation to plow thorugh 37 files of changes. Whats the advantage of this PR?

anntzer · 2018-03-15T19:31:31Z

I would argue that

- def contour(self, X, Y, Z, *args, **kwargs):
+ def contour(self, X, Y, Z, *args,
              extend3d=False, stride=5, zdir='z', offset=None, **kwargs):
<...>
-     extend3d = kwargs.pop('extend3d', False)
-     stride = kwargs.pop('stride', 5)
-     zdir = kwargs.pop('zdir', 'z')
-     offset = kwargs.pop('offset', None)

is a pretty big readability improvement.

jklymak · 2018-03-15T19:36:52Z

Fair, but what does this do? Why is there the extra * in there?

-    def __init__(self, axis, **kwargs):
+    def __init__(self, axis, *, thresh=np.deg2rad(85), **kwargs):

anntzer · 2018-03-15T19:46:05Z

That means that thresh (all arguments after the *) is a keyword-only (kwonly) argument (https://www.python.org/dev/peps/pep-3102/). Taking the simpler example of

def f(x, *, y="foo"): ...

one can call f(1, y=2) but not f(1, 2). In Python2 the only way to achieve this was to use **kwargs and manually pop arguments from the kwargs (which matplotlib did quite often), in Py3 the * is a direct way to get this (which this PR uses).

One not-so-obvious advantage of using kwonly arguments is that it allows us to change the order of kwargs in the doc much more easily (see #7856 for a real-life example).

jklymak · 2018-03-15T19:56:00Z

Thanks - I'll take a quick look soon!

anntzer · 2018-03-29T17:56:13Z

Rebased. This PR caught a spurious kwarg that was added to the example call to clabel() at ec5e886#diff-6585abef3f24dfdde728a606b14e78efR93 and removes it.

Only simple cases (no mutable defaults, no defaults depending on other args) are handled so far.

anntzer added this to the v3.0 milestone Feb 20, 2018

anntzer force-pushed the kwonlyfy branch from dcb7ae7 to d259f8c Compare February 20, 2018 09:58

timhoffm reviewed Feb 20, 2018

View reviewed changes

anntzer force-pushed the kwonlyfy branch 2 times, most recently from 2501cbf to 006cf54 Compare February 20, 2018 16:42

QuLogic added the Py3k label Feb 20, 2018

timhoffm approved these changes Feb 20, 2018

View reviewed changes

anntzer force-pushed the kwonlyfy branch from 006cf54 to b7e5dfd Compare February 22, 2018 03:41

anntzer force-pushed the kwonlyfy branch 2 times, most recently from f4078d3 to edd44da Compare February 22, 2018 03:48

anntzer force-pushed the kwonlyfy branch from edd44da to 1fc7891 Compare March 15, 2018 19:14

anntzer force-pushed the kwonlyfy branch from 1fc7891 to a231104 Compare March 29, 2018 08:11

anntzer force-pushed the kwonlyfy branch from a231104 to 2bfac20 Compare March 29, 2018 17:58

Change manual kwargs popping to kwonly arguments.

a68784f

Only simple cases (no mutable defaults, no defaults depending on other args) are handled so far.

anntzer force-pushed the kwonlyfy branch from 2bfac20 to a68784f Compare April 12, 2018 22:34

jklymak approved these changes Apr 24, 2018

View reviewed changes

jklymak merged commit b01bdf4 into matplotlib:master Apr 24, 2018

anntzer deleted the kwonlyfy branch April 24, 2018 01:13

ngoldbaum mentioned this pull request Aug 13, 2018

fix name of text size keyword in contour callback test yt-project/yt#1970

Merged

Uh oh!

Conversation

anntzer commented Feb 20, 2018

PR Summary

PR Checklist

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Feb 20, 2018

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commented Feb 22, 2018

Uh oh!

anntzer commented Mar 15, 2018

Uh oh!

jklymak commented Mar 15, 2018

Uh oh!

anntzer commented Mar 15, 2018

Uh oh!

jklymak commented Mar 15, 2018

Uh oh!

anntzer commented Mar 15, 2018

Uh oh!

jklymak commented Mar 15, 2018

Uh oh!

anntzer commented Mar 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants