Skip to content

Annotate argument in axes class match upstream #15049

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
Mar 26, 2020

Conversation

ksunden
Copy link
Member

@ksunden ksunden commented Aug 13, 2019

PR Summary

Annotate.__init__ changed it's signature in #13128, however it did not
change the python signature of the downstream functions which inherit its docstring, leading to a mismatch between the docstring and the signature.

I wasn't sure which version should have been put in the decorator, so I used the same as the parent, though I would expect to add the minor version, I guess.

Note: this does change pyplot without having a decorator to catch users who pass in the string explicitly by "s"... Not sure how to handle that

There was some debate in #12383 (comment) and #13128 regarding this variable change, that did mention the effects in pyplot, though seemed to indicate that it was acceptable.

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

@tacaswell tacaswell added this to the v3.2.0 milestone Aug 13, 2019
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

👍 of the change, but can you please add an API change note to https://github.com/matplotlib/matplotlib/tree/master/doc/api/next_api_changes

@tacaswell
Copy link
Member

Good catch!

We should have a bit of a think about what to do about pyplot.annoate though...

@anntzer
Copy link
Contributor

anntzer commented Aug 13, 2019

there's no problem with pyplot.annotate? it'll be subject to the same deprecation period. it's _make_keyword_only that interacts poorly with pyplot.

anntzer
anntzer previously approved these changes Aug 13, 2019
@ksunden
Copy link
Member Author

ksunden commented Aug 13, 2019

>>> plt.annotate(s="hi", xy=(0,0), xytext=(1,1))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: annotate() missing 1 required positional argument: 'text'
>>> plt.gca().annotate(s="hi", xy=(0,0), xytext=(1,1))
__main__:1: MatplotlibDeprecationWarning: The 's' parameter of annotate() has been renamed 'text' since Matplotlib 3.1; support for the old name will be dropped in 3.3.
Text(1, 1, 'hi')

With this patch applied, passing an explicit s kwarg to pyplot.annotate does cause a TypeError.
The function is not decorated with the _rename_parameter decorator.

@anntzer
Copy link
Contributor

anntzer commented Aug 14, 2019

good catch :/
I guess this is another argument in favor of #14130 (comment) (@tacaswell do you want to comment on that?)

if the general problem doesn't get fixed quickly you can always go with manually writing the pyplot wrapper as a stopgap (#14130 (comment)).

@tacaswell tacaswell modified the milestones: v3.2.0, v3.3.0 Aug 24, 2019
@ksunden
Copy link
Member Author

ksunden commented Aug 27, 2019

I'm willing to implement at least the stopgap mentioned by @anntzer, if that is what we want for now

@ksunden
Copy link
Member Author

ksunden commented Feb 26, 2020

I was just reminded of this, I've seen some PRs in recent days/weeks regarding annotate and some of the renaming of params and such, but did not track them closely enough to see if they were fixing this problem in another way, enabling this PR to work as intended, or just actually unrelated to this change entirely.

So, just checking in, seeing if there is anything that needs to be done to this PR to catch up (it appears to me that the docstring and the method signature are still mismatched as of now)

@jklymak
Copy link
Member

jklymak commented Feb 26, 2020

There are no merge conflicts, so thats usually a good sign that no one has touched the section of the code you have touched. OTOH, you have some documentation failures, and should rebase with master to make sure all the CI is using recent CI.

@anntzer
Copy link
Contributor

anntzer commented Feb 26, 2020

Technically this breaks plt.annotate(s=...) without a deprecation, and the fix would be to merge #15254. If we decide to ignore the APi break, I agree (some version of) this should go in ASAP.

@ksunden ksunden force-pushed the annotate_argument branch from e904c07 to 8417223 Compare March 5, 2020 16:04
@ksunden
Copy link
Member Author

ksunden commented Mar 6, 2020

CI Failure looks unrelated to me... (Looks like two image files just failed to write properly, nothing to do with the actual code difference here)

I did test out #15254, the change proposed here is still needed (to Axes.annotate), but that PR does prevent the API deprecation problem. (There is a small merge conflict, as both edit pyplot.py, but it is only in the generated portion of that file, anyway)

@ksunden
Copy link
Member Author

ksunden commented Mar 13, 2020

Closing and opening to rerun CI.

@ksunden ksunden closed this Mar 13, 2020
@ksunden ksunden reopened this Mar 13, 2020
@QuLogic QuLogic linked an issue Mar 20, 2020 that may be closed by this pull request
@ksunden ksunden force-pushed the annotate_argument branch from ebab7e3 to ba34a85 Compare March 22, 2020 02:41
@ksunden
Copy link
Member Author

ksunden commented Mar 22, 2020

Rebased on top of master after #15254 was merged

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Please add a test of the warning.

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

modulo fixing the location of the api_changes.

ksunden and others added 3 commits March 26, 2020 12:16
Run boilerplate.py

change version rename takes effect

Co-Authored-By: Thomas A Caswell <tcaswell@gmail.com>

Release note of variable change

Rebased onto newer master

Address doc building

Apply suggested language change for release note

Co-Authored-By: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Flake8

Match warning to ensure correct one caught, Update version deprecated

Flake8 whitespace

Simplify test
Remove specification for where the old parameter remains supported

This was fixed by merging matplotlib#15254
@ksunden ksunden force-pushed the annotate_argument branch from 6cf8cce to cbc65ac Compare March 26, 2020 17:18
@ksunden
Copy link
Member Author

ksunden commented Mar 26, 2020

Rebased on top of master to avoid merge conflict

@QuLogic QuLogic merged commit a032c6f into matplotlib:master Mar 26, 2020
@ksunden ksunden deleted the annotate_argument branch March 26, 2020 22:10
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.

small typo in the documentation
6 participants