Use more kwonly arguments, less manual kwargs-popping. #15680

anntzer · 2019-11-12T13:57:54Z

PR Summary

PR Checklist

Has Pytest style unit tests
Code is Flake 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

lib/matplotlib/artist.py

lib/matplotlib/axes/_axes.py

lib/matplotlib/axes/_base.py

QuLogic · 2019-11-13T04:08:46Z

lib/mpl_toolkits/axisartist/floating_axes.py

        grid_helper = self.get_grid_helper()
+        if not hasattr(grid_helper, "get_boundary"):
+            raise ValueError("grid_helper must implement get_boundary method")


Is it really helpful to move this into a private method? Seems clearer for a user if the exception comes from the class init directly.

_gen_axes_patch is called immediately at init so users would see the message immediately, just with a few more stack frames. Putting the check e.g. after the super()init call would actually be useless because super()init would already have called _gen_axes_patch and thus already errored out.

I don't understand. The first thing the original code did in __init__ was validating the grid_helper parameter.

lib/matplotlib/axes/_axes.py

anntzer · 2019-11-17T19:05:29Z

For indicate_inset, I fixed the doc issue by adding a property table to the docstring. For inset_axes that's not possible because at the time the method is defined, the Axes class doesn't actually exist yet so has no entry in the interpd map. I'm not sure it's really worth fighting the docstring interpolation system to fix that and I'm not sure I can write a meaningful doc for that parameter (label: the axes label is actually confusing, as it's not the x/y label; a longer explanation would be distracting from the main things that the docstring of inset_axes should be explaining) @timhoffm I can revert that specific change if you'd like.

timhoffm

I'd prefer reverting the changes to the label parameter. AFAICS this is not intended as a user-facing parameter and thus should better not appear in the signature.

lib/matplotlib/axes/_base.py

timhoffm · 2019-11-24T16:50:50Z

lib/mpl_toolkits/axisartist/floating_axes.py

        grid_helper = self.get_grid_helper()
+        if not hasattr(grid_helper, "get_boundary"):
+            raise ValueError("grid_helper must implement get_boundary method")


I don't understand. The first thing the original code did in __init__ was validating the grid_helper parameter.

anntzer · 2019-11-24T18:51:39Z

Re: label: reverted, put in a small variation on the handling.
Re: axes_locator: Indeed I can remove it from the explicit list and just init self._axes_locator = None.
Re: floating_axes: It seems a bit silly to do manual kwargs handling in the init just for validation purpose, when the function call machinery will already match kwargs with parameters for us. Now the question is just where to put the validation, and _gen_axes_patch (which is called by the super() ini 8000 t call) is the only place to do it.

timhoffm · 2019-11-24T19:27:31Z

It seems a bit silly to do manual kwargs handling in the init just for validation purpose, when the function call machinery will already match kwargs with parameters for us.

This is not fundamentally different from other parameter checks. The checks are in place so that we can error out from a place close to the user call with a descriptive error message.

anntzer · 2019-11-24T19:47:24Z

The error message is just as descriptive, and will be triggered in the init call (or more specifically two frames below it, but the user will need to go up the traceback to check where their own incorrect code is anyways).

Also, consider for example a (hypothetical) subclass of FloatingAxes for which the grid_helper would be pre-set to some default value -- making the grid_helper kwarg optional (or even not available). With the earlier design, one would additionally need to override init to undo the currently existing check. With this PR's design one doesn't need to do anything. Hence I would consider this PR's design more composable.

timhoffm · 2019-11-24T21:02:24Z

The error message is just as descriptive, and will be triggered in the init call

I can anticipate that for "grid_helper must implement get_boundary method" but not for "FloatingAxes requires grid_helper argument".

Also, consider for example a (hypothetical) subclass of FloatingAxes for which the grid_helper would be pre-set to some default value -- making the grid_helper kwarg optional (or even not available).

Generally, if you change the signature of a function, it's reasonable that it should be reimplemented. Having parameters just *args, **kwargs and determining which ones to accept from some remote destination is a bit too much magic to my taste.

A major problem with this part of the code is that it's very indirect and largely undocumented. I cannot judge the effect of changes and therefore I'm unable to review that part. If you're attached to the changes, would it be an option to move that part to another PR so that I an approve the rest of this PR and we hopefully find somebody else to review the floating axes stuff.

anntzer · 2019-11-24T21:08:58Z

I reverted that change (except for the docstring unduplication).

timhoffm

Modulo a minor comment.

lib/matplotlib/axes/_base.py

anntzer · 2020-03-24T08:46:44Z

rebased

timhoffm · 2020-03-24T10:10:23Z

Doc failure:

WARNING: /home/circleci/project/examples/subplots_axes_and_figures/zoom_inset_axes.py failed to execute correctly: Traceback (most recent call last):
  File "/home/circleci/.local/lib/python3.6/site-packages/sphinx_gallery/gen_rst.py", line 440, in _memory_usage
    out = func()
  File "/home/circleci/.local/lib/python3.6/site-packages/sphinx_gallery/gen_rst.py", line 425, in __call__
    exec(self.code, self.globals)
  File "/home/circleci/project/examples/subplots_axes_and_figures/zoom_inset_axes.py", line 33, in <module>
    axins = ax.inset_axes([0.5, 0.5, 0.47, 0.47])
  File "/home/circleci/project/lib/matplotlib/axes/_axes.py", line 460, in inset_axes
    inset_ax = Axes(self.figure, bb.bounds, zorder=zorder, label=label,
NameError: name 'label' is not defined

anntzer · 2020-03-24T10:27:55Z

oops, hopefully fixed...

anntzer 8000 added the Maintenance label Nov 12, 2019

anntzer force-pushed the kwonly branch 4 times, most recently from 6ada6c4 to 3353e74 Compare November 12, 2019 16:18

QuLogic reviewed Nov 13, 2019

View reviewed changes

anntzer force-pushed the kwonly branch 2 times, most recently from 1964586 to fa7b508 Compare November 13, 2019 12:02

timhoffm reviewed Nov 16, 2019

View reviewed changes

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

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

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

anntzer force-pushed the kwonly branch from fa7b508 to c944ca7 Compare November 17, 2019 19:05

timhoffm reviewed Nov 24, 2019

View reviewed changes

anntzer force-pushed the kwonly branch from c944ca7 to c985ab2 Compare November 24, 2019 18:52

anntzer force-pushed the kwonly branch from c985ab2 to 8b4aa0f Compare November 24, 2019 21:08

timhoffm approved these changes Nov 25, 2019

View reviewed changes

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

anntzer force-pushed the kwonly branch from 8b4aa0f to 8702500 Compare November 26, 2019 08:35

anntzer mentioned this pull request Jan 9, 2020

Numpydocify artist.getp(). #16174

Merged

6 tasks

QuLogic added the status: needs rebase label Mar 24, 2020

QuLogic approved these changes Mar 24, 2020

View reviewed changes

tacaswell added this to the v3.3.0 milestone Mar 24, 2020

anntzer removed the status: needs rebase label Mar 24, 2020

anntzer force-pushed the kwonly branch from 8702500 to 694850b Compare March 24, 2020 08:46

Use more kwonly arguments, less manual kwargs-popping.

df7661b

anntzer force-pushed the kwonly branch from 694850b to df7661b Compare March 24, 2020 10:27

timhoffm merged commit 8ba451b into matplotlib:master Mar 24, 2020

anntzer deleted the kwonly branch March 24, 2020 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Use more kwonly arguments, less manual kwargs-popping. #15680

Use more kwonly arguments, less manual kwargs-popping. #15680

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Use more kwonly arguments, less manual kwargs-popping. #15680

Use more kwonly arguments, less manual kwargs-popping. #15680

Uh oh!

Conversation

PR Summary

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!