-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Explicit args and refactor Axes.margins #11146
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
Conversation
52509b7
to
da5f85b
Compare
lib/matplotlib/axes/_base.py
Outdated
@@ -2229,7 +2229,7 @@ def set_ymargin(self, m): | |||
self._ymargin = m | |||
self.stale = True | |||
|
|||
def margins(self, *args, **kw): | |||
def margins(self, x_or_both=None, y=None, *, x=None, tight=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like a terrible signature (enough that I am willing to block on it). Yes it's too bad that so many functions can take either a tuple or two arguments, but that's what we have to work with.
I would just keep it (x=None, y=None) and document in the docstring that x
can also be "both". If not, even the original *args, **kwargs
seems less terrible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like a terrible signature (enough that I am willing to block on it).
I agree! This is a awful API! Unfortunately, it's also the exact API that exists right now.
Yes it's too bad that so many functions can take either a tuple or two arguments, but that's what we have to work with. I would just keep it (x=None, y=None) and document in the docstring that x can also be "both".
x_or_both
(better names welcome) is always a scalar; it's not one of those that can be a tuple of scalars. If only one positional argument is provided, that argument is used for both x
and y
- so if we call the first argument x
, it becomes impossible to specify (x=1, y=None)
😭
If not, even the original *args, **kwargs seems less terrible.
I don't like this interface, but I think hiding it behind *args, **kwargs
is even worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API is quite unpythonic in the sense that it cannot be easily expressed as an explicit function signature. Unfortunately, we have to live with that.
We're mainly changing to explicit signatures so that they are easier to understand for the user. IMO this is not the case for the proposed signature.
In this case, I would just go for margins(*args, *, tight=True, *kw)
And describe the possible signatures in a Call signatures
block in the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider the new signature to have negative informativity. At least with the old one you just accept that you have to read the docstring and move on...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the proposed API is pretty confusing.
Is one or two positional args really unpythonic? I agree that I hate *args
as much as the next person, but sometimes its useful, as is being able to pop a kwarg to see if it was passed. Would the pythonic way of doing this just be to force both positional args to be passed with the same value? (Not being argumentative, truly curious how this can/should be done in future).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry. I was confusing the situation with xlim(), which can take xlim(a, b)
or xlim((a, b))
.
Here what we really have is broadcasting, which is a bit different. I can't say I really like any of the options, with still a mild preference for *args, **kwargs
, but I guess the proposal is not as bad as what I thought :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jklymak I would say that one or two positional args is not unpythonic per se. What's really making things difficult is that the semantics of argument i depends on the number of arguments. This implies that you cannot give it a reasonable explicit name in the signature.
Other languages can do this by method overloading. However, Python does not support method overloading because it's not needed for the typical use cases that require method overloading in other languages. This use case here may be one of the few examples, where actually method overloading could add a benefit in Python.
You really have two semantically different signatures for the same functions. That's rarely what you want (and probably also a reason Python doesn't have method overloading), but I sort of see the point here.
Since we only have one signature to state, it should not give raise to misleading code. All these calls should work:
margins(margin)
margins(xmargin, ymargin)
margins(x=xmargin)
margins(y=ymargin)
margins(x=xmargin, y=ymargin)
In particular, the first three cannot be fulfilled simultaneously with a single explicit signature. Therefore, I've propsed above to keep *args and *kw for this mechanism and document the possibilities in the docstring.
Explicit arguments instead of \*args, \*\*kwargs | ||
------------------------------------------------ | ||
|
||
:PEP:`3102` describes keyword-only arguments, which allow Matplotlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure referring a 10 (or so) year old PEP is really helpful, but ymmv :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linked mostly for the motivations section, and to provide context that this is a Py3-only feature. MM does V, but happy to edit if requested.
Hi all! There's some great discussion here, which I will summarise as:
I'd therefore like to suggest an alternative: def margins(self, *margins, x=None, y=None, tight=True): This could also match the current behaviour, but I'd actually make a minor change - make it an error to pass both positional and keyword arguments for a margin, rather than positional arguments silently overriding keyword arguments. Ie: Thoughts? If nobody objects I'll implement this later today 😄 |
Sounds good. Nevertheless, this needs some explanation in the docstring. |
Now implemented - and explained in all of docstrings, exception messages, and API change notes. |
... though looks like PEP8 is mad at you... |
Gah, new computer didn't have my editor configured to strip trailing whitespace on save. Fixed. |
🎉 Step by painful step, the API improves 🚀 |
Thanks for your work! 👍 |
Happy to help out! I've run up against something confusing in Matplotlib and bounced off hard a few times myself, and watched it happen in more python-for-scientists workshops than I care to count. Anything I can do to help directly (eg this) or make contributing easier (eg refactoring) seems worth doing if I have the time, and I've been in a cleaning-up mood lately... the mood will pass, but the code will endure 😄 |
That's actually what brought me here as well. 😄 |
This pull contains part of my follow-up to #11137, likewise addressing #9912.
Axes.margins
had a really complicated way of handling*args, **kwargs
, which it didn't need at all. The new signature is fully backwards-compatible, unless you previously relied on unknown kwargs being ignored, and much easier to understand.