Repository navigation
Cleaning up variable argument signatures #9912
Description
There are a lot of *args, **kwargs signatures in the API. In many cases, they could be better described by explicit arguments/kw-arguments. This makes for a more clear API and reduces the chance of unintended misuse.
For example pyplot.delaxes
def delaxes(*args):
"""..."""
if not len(args):
ax = gca()
else:
ax = args[0]
...
would be more clear with
def delaxes(ax=None):
"""..."""
if ax is None:
ax = gca()
...
For all intended uses, the API compatibility is preserved. Unreasonable and possibly wrong uses like
delaxes(ax1, ax2) or delaxes(ax, mykwarg=True) would raise an Exception after the change. In itself, this is a benefit, because these cases will then be detected as false usage. However it may break existing programs.
Before I do any pull requests, is there a policy how to handle such cases?
- Do you want to change these things, or should they be left as they are:
- for cases like the above one where arguments are directly resolved in the function/method?
- for cases where the all the arguments are passed like e.g.
pyplot.savefig?
- Which branch should changes go into?
Activity
afvincent commentedon Dec 2, 2017
@timhoffm Just tagged the ticket with a bird call for @tacaswell and the other core devs 😈, so you should receive attention ;).
jklymak commentedon Dec 2, 2017
Not a core dev, but...
For your two cases, I think "yes" to locally resolved arguments and "no" to ones that are passed through.
I just took a quick look through
axes/_axesand the places where positional arguments are not specified tends to be limited to methods where the arguments are passed through to another function. i.e.matplotlib/lib/matplotlib/axes/_axes.py
Line 1244 in 85e124a
I imagine cases like
delaxesare fine to clean up. But those that just passthrough should probably be left alone: i.e.matplotlib/lib/matplotlib/pyplot.py
Line 1283 in 85e124a
I agree the downstream code in the case of passthroughs should disallow illegal inputs, and I think in most cases it does, though perhaps we don't bother checking for extra useless kwargs all the time.
We always work off master branch. I've been active for a few months and only seen one person base a PR off another branch ;-)
tacaswell commentedon Dec 5, 2017
This is definitely not bug fixes so master branch.
For functions / methods which are the bottom of the a call stack should ensure that they consume everything, but for practical reasons am wary of blowing up all of the instance of
**kwargsto explicit keywords. The verbosity and the many layers of defaults would be a nasty maintenance burden very quickly.I would also hold off on making anything that isn't already positional, positional (When we get to drop python2 we can make things currently burried in kwargs key-word only 😄 ). That is extending the API in a more brittle direction.
(the above is mostly just re-saying what @jklymak said)
timhoffm commentedon Dec 5, 2017
Thanks for your comments. I think it's a reasonable strategy, not to replicate full signatures. Nevertheless, I would like to propose one special case for discussion:
Positional arguments of
pyplotfunctions that directly map to positional arguments in Figure/Axes methodsExample:
pyplot.savefig(*args, **kwargs)and
Figure.savefig(self, fname, **kwargs)IMO it would be reasonable for this and similar cases to explicitly write out the positional arguments in pyplot, i.e.
pyplot.savefig(fname, **kwargs).Similar cases:
figtext,suptitle,xscale,yscale,xtics,yticks, ...Motivation
The explicit arguments helps a great deal in understanding the basic use case. It may appear in code completion and help.
Discussion
*argshas to resolve exactly tofnamein the above example). We just make this explicit.timhoffm commentedon Dec 9, 2017
@tacaswell @jklymak I would like to hear your view on the above suggestion concerning positional arguments. Thanks.
jklymak commentedon Dec 9, 2017
My opinion doesn't count too much, largely because I'm not very much of a pythonista. But I agree that a couple of repeated positional arguments is entirely reasonable, and it makes the pyplot routine's docstrings more accessible.
Zac-HD commentedon Jan 9, 2018
This issue should be on the v3.0 milestone!
This will be really nice for anyone who treats function signatures as usage hints - which includes many users of docs, IPython and Jupyter, etc. I'm certainly excited!
Clever idea that might help with this:
**kwargsfor any arguments that they consume.**kwargsare reserved for things passed through to another function.passes_kwargs_to(lower)(upper)which raises an error if kwargs are passed to upper which are not used by lower (including by passing toeven_lower, etc).**kwargsinto kwonly args in the runtime argspec, or just validate that kwargs contain only useful keys when the function is called. Personally I favor the latter version - it's much nicer to read in the docs, but still useful at runtime.Somebody ping me when there's a branch that doesn't support Python 2, and I'd be happy to write the patch 😄
tacaswell commentedon Jan 9, 2018
@Zac-HD You can start writing that against master, just won't be merged until we tag 2.2 (later this month).
How much over-head would such a decorator add? We have some messy things that may pass to more than one and in many cases, the final consumer of kwargs is the
setpmethod which will blow up on un-known kwargs so I am not sure that raising a bit earlier is enough of a win for the added complexity of wrapping all of our methods/functions in another layer of decorator.Zac-HD commentedon Apr 27, 2018
Idea update: IMO a decorator or similar just isn't worth the effort; any large-scale editing effort would be better off fixing general style and structure issues. I've done a chunk of manual cleanup and opened a PR instead 😄
I plan to do a short series of these, on the basis that incremental improvements that get merged are better than big changes which never turn up!
Hey, how many places (as of 17b6c30) do you think are using
**kwargsbut should be using named keyword-only arguments? This search isn't perfect, but it is suggestive:What other searches might be useful? Note that "useful" here really means "finds something we should fix, with low false-positive rate" 😉
For example, it's idiomatic to use sequences directly in a boolean context, ie
if args:is better thanif len(args):.In the course of writing out the argspec for
ax.semilogy, it turns out that the default behaviour was incorrectly documented - non-positive coordinates are clipped to just above zero, rather than masked. This was probably copied fromsemilogx(which does mask), and the docs not updated when someone changed the behaviour.This should probably be treated as a bug, and the behaviour for y changed to match x.
anntzer commentedon May 18, 2018
Closing as #11145 and #11146 should already take care of many cases and we can always merge more PRs on the same topic on a piecemeal fashion.