Skip to content

Change manual kwargs popping to kwonly arguments. #10545

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

Merged
merged 1 commit into from
Apr 24, 2018

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 20, 2018

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

@anntzer anntzer added this to the v3.0 milestone Feb 20, 2018
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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rightside_up missing

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

@@ -790,8 +790,9 @@ class FigureCanvasPgf(FigureCanvasBase):
def get_default_filetype(self):
return 'pdf'

def _print_pgf_to_fh(self, fh, *args, **kwargs):
if kwargs.get("dryrun", False):
def _print_pgf_to_fh(self, fh, *args,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*args is unused -> * only

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

# whether the arguments are iterable, and if so we try and convert them
# to a tuple. However, the ``iterable`` function returns True if
# __getitem__ is present, but some classes can define __getitem__ without
# being iterable. The tuple conversion is now done in a try...except in
# case it fails.

class MyAxes(Axes):
def __init__(self, *args, **kwargs):
kwargs.pop('myclass', None)
def __init__(self, *args, myclass=None **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing comma

plt.draw_if_interactive()
return ax

def host_subplot(*args, **kwargs):
def host_subplot(*args, axes_class=None, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing figure kwarg

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

anntzer commented Feb 20, 2018

thanks, comments handled

@anntzer anntzer force-pushed the kwonlyfy branch 2 times, most recently from 2501cbf to 006cf54 Compare February 20, 2018 16:42
@QuLogic QuLogic added the Py3k label Feb 20, 2018
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

anntzer commented Feb 22, 2018

rebased

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

anntzer commented Mar 15, 2018

rebased

@jklymak
Copy link
Member

jklymak commented Mar 15, 2018

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

@anntzer
Copy link
Contributor Author

anntzer commented Mar 15, 2018

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
Copy link
Member

jklymak commented Mar 15, 2018

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
Copy link
Contributor Author

anntzer commented Mar 15, 2018

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
Copy link
Member

jklymak commented Mar 15, 2018

Thanks - I'll take a quick look soon!

@anntzer
Copy link
Contributor Author

anntzer commented Mar 29, 2018

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.
@jklymak jklymak merged commit b01bdf4 into matplotlib:master Apr 24, 2018
@anntzer anntzer deleted the kwonlyfy branch April 24, 2018 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants