Skip to content

Parameter-renaming decorator #13128

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 3 commits into from
Jan 28, 2019
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 7, 2019

PR Summary

xref #13117 (comment), #11070, #12563, #12383.

attn @tacaswell @timhoffm I guess.

Edit: hum, probably needs a bit more thought to handle the case of **kwargs silently grabbing all unknown kwargs...
Edit: fixed.

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

@anntzer anntzer changed the title Rename parameter Parameter-renaming decorator Jan 7, 2019
@anntzer
Copy link
Contributor Author

anntzer commented Jan 7, 2019

Actually @efiring argued in #12383 (comment) against the s->text change; I personally don't really mind either way and can revert it here if that's the agreement.
Note, though, that right now it's plt.text(x, y, s) but Text(x, y, text); they should be made consistent.

@tacaswell tacaswell added this to the v3.1 milestone Jan 7, 2019
@tacaswell
Copy link
Member

Looks like it interacts with pyplot in a bit of a weird way too...

@efiring
Copy link
Member

efiring commented Jan 7, 2019

Regarding my earlier comment in favor of "s": I'm willing to back off on that. It probably does make more sense at this point for annotate to match the Text API and use 'text'.

@@ -6903,7 +6905,7 @@ def hist2d(self, x, y, bins=10, range=None, normed=False, weights=None,
"""

h, xedges, yedges = np.histogram2d(x, y, bins=bins, range=range,
normed=normed, weights=weights)
normed=density, weights=weights)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be "density=density"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's called normed on hist2d side until np 1.15... #11070 (comment)

@anntzer
Copy link
Contributor Author

anntzer commented Jan 7, 2019

I see your point @tacaswell re: pyplot. I think everything can be made to work, just needs a bit more care...

@anntzer
Copy link
Contributor Author

anntzer commented Jan 8, 2019

Went for a different, and in fact much simpler implementation (turns out the pyplot wrappers really constrain the design space here).

QuLogic
QuLogic previously requested changes Jan 9, 2019
@tacaswell
Copy link
Member

Could you also add a make_paramaters_after_N_kwarg_only as a decorator?

@anntzer
Copy link
Contributor Author

anntzer commented Jan 13, 2019

That's actually going to be harder wrt. the pyplot wrappers, I think(?), because that decorator probably really needs to be applied on the wrapper as well (as the wrappers pass everything down by keyword, so the inner function cannot check whether the argument was passed positionally to the wrapper), and I tried quite hard (both here and in #13173) to not "store additional information" on the method to tell whether the wrapper needs additional decorators.

Edit: IIRC we can't make the pyplot wrappers just take *args, **kwargs and forward them as such to the axes methods because jedi/other autocompletion tools would hate us, but I guess we can add a decorator around each pyplot wrapper of the form

@pyplot_wrapper(gca, Axes.text)
def text(x, y, s, ...): return gca().text(x, y, s, ...)

with

def pyplot_wrapper(gca, method):
    def decorator(func):
        @functools.wraps(func)
        def wrapper(*args, **kwargs): return method(gca(), *args, **kwargs)
        return wrapper
    return decorator

i.e. the decorator actually completely ignores the implementation of the wrapper and directly forwards the arguments to the method instead... now the method will know whether the argument was passed positionally or as keyword, and the "deprecate_passing_positionally" decorator will work.
Ugly, isn't it :)

@tacaswell
Copy link
Member

I don't think it would be terrible for the pyplot generating code to be aware of our deprecation decorators to a sufficient degree that it could re-write them into the generated code. That would be nice for the humans who look at the code in pyplot.

On the other hand, reducing the bodies of the functions in pyplot to ... also seems appealing...

@anntzer
Copy link
Contributor Author

anntzer commented Jan 14, 2019

Now that I think of it, one main argument for moving to keyword-only arguments is to make it easier to rename or to delete arguments, but now that this PR and #13173 implement these features, I guess it's not that useful anymore?

@jklymak jklymak requested a review from tacaswell January 15, 2019 16:59
@jklymak
Copy link
Member

jklymak commented Jan 15, 2019

This seems fine to me, but I don't think it should be merged unless @tacaswell and/or @efiring approve.

one main argument for moving to keyword-only arguments is to make it easier to rename or to delete arguments

I guess thats true, but I think its also better to make the interface explicit to get rid of the magic guessing we do about what each arg means.

@jklymak jklymak requested a review from efiring January 16, 2019 17:00
Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me. It affects only a few places for now, so if some horrible disadvantage of it turns up, or a much better approach is found, it will be easy to revert. I don't expect that, though. It looks pretty clean and simple.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 20, 2019

Good to merge? :)

@jklymak
Copy link
Member

jklymak commented Jan 20, 2019

I'd like @tacaswell to merge.

I think we should also decide some sort of guidelines for renaming... Just because this is easier dosn't mean we should break old documentation and examples on webpages...

@anntzer
Copy link
Contributor Author

anntzer commented Jan 20, 2019

I think it should be treated as an API break like most others, although with a lower bar if we can reasonably assume that the parameter was rarely if ever passed as keyword (e.g. matplotlib.use(arg=...) is probably rare, whereas hist(normed=...) is fairly common (relative to the number of uses of hist())).

@jklymak
Copy link
Member

jklymak commented Jan 25, 2019

ping @tacaswell - I'll merge if don't hear from you by w/e

@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2019

(force push: replaced "argument" by "parameter" in the changelog, for consistency with the decorator name, see also https://stackoverflow.com/questions/156767/whats-the-difference-between-an-argument-and-a-parameter)

@tacaswell tacaswell merged commit 114d516 into matplotlib:master Jan 28, 2019
@anntzer anntzer deleted the rename_parameter branch January 28, 2019 20:18
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.

6 participants