NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

jklymak merged 1 commit intomatplotlib:masterfromanntzer:kwonlyfy

Apr 24, 2018

Merged

Change manual kwargs popping to kwonly arguments.#10545

jklymak merged 1 commit intomatplotlib:masterfromanntzer:kwonlyfy

Apr 24, 2018

Conversation

Copy link

Contributor

anntzer commentedFeb 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 added this to thev3.0 milestone

Feb 20, 2018

anntzer force-pushed thekwonlyfy branch fromdcb7ae7 tod259f8cCompare

February 20, 2018 09:58

timhoffm reviewed

Feb 20, 2018

View reviewed changes

lib/matplotlib/contour.py Outdated

		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

timhoffm reviewed

Feb 20, 2018

View reviewed changes

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

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

ContributorAuthor

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.

lib/matplotlib/backends/backend_pgf.py

		@@ -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

ContributorAuthor

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.

lib/matplotlib/tests/test_figure.py Outdated

		# 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

lib/mpl_toolkits/axes_grid1/parasite_axes.py Outdated

		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

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):

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

ContributorAuthor

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 ororientation really is.

When doing

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

it finally raises aKeyError onax.axis[self.orientation].toggle(all=b) inCbarAxesBase._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.

Copy link

ContributorAuthor

anntzer commentedFeb 20, 2018

thanks, comments handled

anntzer force-pushed thekwonlyfy branch 2 times, most recently from2501cbf to006cf54Compare

February 20, 2018 16:42

QuLogic added the Py3k label

Feb 20, 2018

timhoffm approved these changes

Feb 20, 2018

View reviewed changes

Copy link

Member

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 force-pushed thekwonlyfy branch from006cf54 tob7e5dfdCompare

February 22, 2018 03:41

Copy link

ContributorAuthor

anntzer commentedFeb 22, 2018

rebased

anntzer force-pushed thekwonlyfy branch 2 times, most recently fromf4078d3 toedd44daCompare

February 22, 2018 03:48

anntzer force-pushed thekwonlyfy branch fromedd44da to1fc7891Compare

March 15, 2018 19:14

Copy link

ContributorAuthor

jklymak commentedMar 15, 2018

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

Copy link

ContributorAuthor

anntzer commentedMar 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.

Copy link

Member

anntzer commentedMar 15, 2018

That means thatthresh (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 callf(1, y=2) butnotf(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).

Copy link

Member

jklymak commentedMar 15, 2018

Thanks - I'll take a quick look soon!

anntzer force-pushed thekwonlyfy branch from1fc7891 toa231104Compare

March 29, 2018 08:11

Copy link

ContributorAuthor

anntzer commentedMar 29, 2018

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

anntzer force-pushed thekwonlyfy branch froma231104 to2bfac20Compare

March 29, 2018 17:58

Change manual kwargs popping to kwonly arguments.

a68784f

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

anntzer force-pushed thekwonlyfy branch from2bfac20 toa68784fCompare

April 12, 2018 22:34

jklymak approved these changes

Apr 24, 2018

View reviewed changes

jklymak merged commitb01bdf4 intomatplotlib: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 testyt-project/yt#1970

Merged

Labels

None yet

4 participants

Movatterモバイル変換

Uh oh!

Change manual kwargs popping to kwonly arguments.#10545

Change manual kwargs popping to kwonly arguments.#10545

Uh oh!

Conversation

anntzer commentedFeb 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 commentedFeb 20, 2018

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commentedFeb 22, 2018

Uh oh!

anntzer commentedMar 15, 2018

Uh oh!

jklymak commentedMar 15, 2018

Uh oh!

anntzer commentedMar 15, 2018

Uh oh!

jklymak commentedMar 15, 2018

Uh oh!

anntzer commentedMar 15, 2018

Uh oh!

jklymak commentedMar 15, 2018

Uh oh!

anntzer commentedMar 29, 2018

Uh oh!

Uh oh!