Skip to content

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

Merged
merged 1 commit into from
May 2, 2018

Conversation

Zac-HD
Copy link
Contributor

@Zac-HD Zac-HD commented Apr 30, 2018

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.

  • Code is PEP 8 compliant
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@Zac-HD Zac-HD force-pushed the refactor-margins branch 2 times, most recently from 52509b7 to da5f85b Compare April 30, 2018 12:48
anntzer
anntzer previously requested changes May 1, 2018
@@ -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):
Copy link
Contributor

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.

Copy link
Contributor Author

@Zac-HD Zac-HD May 1, 2018

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.

Copy link
Member

@timhoffm timhoffm May 1, 2018

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.

Copy link
Contributor

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

Copy link
Member

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

Copy link
Contributor

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

Copy link
Member

@timhoffm timhoffm May 1, 2018

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

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

Copy link
Contributor Author

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.

@anntzer anntzer dismissed their stale review May 1, 2018 17:54

was confused

@Zac-HD
Copy link
Contributor Author

Zac-HD commented May 2, 2018

Hi all! There's some great discussion here, which I will summarise as:

  • Nobody is very happy about the existing API, because the meaning of the positional arguments depends on how many are given.
  • The signature (self, x_or_both=None, y=None, *, x=None, tight=True) exposes this but is super ugly.

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: margins(0, tight=False) is OK but margins(0, x=1) is an error.

Thoughts? If nobody objects I'll implement this later today 😄

@timhoffm
Copy link
Member

timhoffm commented May 2, 2018

Sounds good. Nevertheless, this needs some explanation in the docstring.

@Zac-HD Zac-HD force-pushed the refactor-margins branch from da5f85b to 8dc5310 Compare May 2, 2018 03:01
@Zac-HD
Copy link
Contributor Author

Zac-HD commented May 2, 2018

Now implemented - and explained in all of docstrings, exception messages, and API change notes.

@jklymak
Copy link
Member

jklymak commented May 2, 2018

... though looks like PEP8 is mad at you...

@Zac-HD Zac-HD force-pushed the refactor-margins branch from 8dc5310 to 17e60a4 Compare May 2, 2018 08:03
@Zac-HD
Copy link
Contributor Author

Zac-HD commented May 2, 2018

... though looks like PEP8 is mad at you...

Gah, new computer didn't have my editor configured to strip trailing whitespace on save. Fixed.

@dstansby dstansby merged commit bc2ea01 into matplotlib:master May 2, 2018
@dstansby dstansby added this to the v3.0 milestone May 2, 2018
@Zac-HD Zac-HD deleted the refactor-margins branch May 2, 2018 13:29
@Zac-HD
Copy link
Contributor Author

Zac-HD commented May 2, 2018

🎉

Step by painful step, the API improves 🚀

@timhoffm
Copy link
Member

timhoffm commented May 2, 2018

Thanks for your work! 👍

@Zac-HD
Copy link
Contributor Author

Zac-HD commented May 2, 2018

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 😄

@timhoffm
Copy link
Member

timhoffm commented May 2, 2018

That's actually what brought me here as well. 😄

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.

5 participants