Skip to content

Make pyplot more tolerant wrt. 3rd-party subclasses.#12293

Merged
jklymak merged 2 commits intomatplotlib:masterfrom
anntzer:tolerant-pyplot
Oct 6, 2018
Merged

Make pyplot more tolerant wrt. 3rd-party subclasses.#12293
jklymak merged 2 commits intomatplotlib:masterfrom
anntzer:tolerant-pyplot

Conversation

@anntzer
Copy link
Copy Markdown
Contributor

@anntzer anntzer commented Sep 26, 2018 •
edited
Loading

Don't force them to reproduce the names of "intended-as-positional"
arguments; don't force them to support the data kwarg.

xref #12288

attn @QuLogic can you pick it up from there if more changes are needed?

PR Summary

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 added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Sep 26, 2018
@anntzer anntzer added this to the v3.0.x milestone Sep 26, 2018
@anntzer anntzer force-pushed the tolerant-pyplot branch 2 times, most recently from 0755ea7 to 7ddf5fd Compare September 26, 2018 08:35
jklymak
jklymak previously requested changes Sep 26, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

The fact that all this didn't trip any test changes means to me that we aren't testing the signatures completely, and that is making breaking changes opaque to us, but a problem for downstream packages. If we decide to change the signatures, that should fail some tests, which can then consciously be removed if need be, and accompanied by the appropriate API change notes. (i.e. the changes made in #10918 shoudl have failed some tests so we knew we were causing problems, so this is the time to add those tests)

@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Sep 26, 2018

It's going to be pretty tricky to test this unless we make a copy of all pyplot signatures somewhere else to compare against, which sounds not so nice.

@jklymak
Copy link
Copy Markdown
Member

jklymak commented Sep 26, 2018

I don’t think they all have to be tested just a smattering to make sure the boilerplate is doing the right thing.

I do t understand what you mean by copying all the pyot signatures but maybe I’m misunderstanding what is going on here

@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Sep 26, 2018

It's not clear to me what test you exactly want to add either...

@jklymak
Copy link
Copy Markdown
Member

jklymak commented Sep 27, 2018

Ideally there would be some tests that would have failed #10918, which presumably means duplicating whatever signature cartopy was trying to use. But again if there is a more informed critical mass of devs who don't think this is possible they should feel free to dismiss my review. I'm not claiming deep understanding

@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Sep 27, 2018

I have not gone through the changes, but this is sufficient to get Cartopy tests working again. Keep in mind that Cartopy does not of course exercise the entirety of the pyplot API, just the following:

$ rg --no-filename -o 'plt\.[A-Za-z0-9_]+' | sort -u
plt.autoscale
plt.axes
plt.clf
plt.close
plt.contour
plt.contourf
plt.draw
plt.figure
plt.gca
plt.gcf
plt.get_backend
plt.imread
plt.imshow
plt.pcolor
plt.pcolormesh
plt.plot
plt.savefig
plt.scatter
plt.show
plt.subplot
plt.subplots_adjust
plt.switch_backend
plt.xlim
plt.ylim

@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Sep 27, 2018 •
edited
Loading

TBH I am not particularly interested in figuring out how to do implement a test (especially wrt what guarantees we want to make regarding the subclassability of Axes).

@jklymak
Copy link
Copy Markdown
Member

jklymak commented Sep 28, 2018

TBH I am not particularly interested in figuring out how to do implement a test (especially wrt what guarantees we want to make regarding the subclassability of Axes).

I think all I'm asking for are tests that test the allowed signatures, which I think is where the downstream subclasses are tripping up. But again, anyone who understands this better can dismiss my review if I'm off base about the testability of this.

@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Sep 29, 2018

Can you open the request for tests as a separate issue instead?

@anntzer anntzer mentioned this pull request Oct 5, 2018
Closed
@jklymak jklymak dismissed their stale review October 5, 2018 21:26

I still think we should test these code paths so we don't break things like this in the future.

@anntzer
Make pyplot more tolerant wrt. 3rd-party subclasses.
7809b91 
Don't force them to reproduce the names of "intended-as-positional"
arguments; don't force them to support the data kwarg.

This is also necessary for methods that take `*args`, as otherwise
we generate a call of the form `foo=foo, *args` which is incorrectly
understood as `*args, foo=foo`.
@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Oct 6, 2018

Clarified in the commit message why this also closes #12405.

dstansby
dstansby previously requested changes Oct 6, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

I think it's reasonable to request at least one small smoke test be added to this PR to check that all is working. The short example in #12405 should be enough

@anntzer
Copy link
Copy Markdown
Contributor Author

anntzer commented Oct 6, 2018

Can you push it?

@dstansby
Copy link
Copy Markdown
Member

dstansby commented Oct 6, 2018

Done!

@dstansby dstansby dismissed their stale review October 6, 2018 15:31

Test added

QuLogic
QuLogic approved these changes Oct 6, 2018
View reviewed changes
Copy link
Copy Markdown
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.

Works for Cartopy.

@jklymak
Copy link
Copy Markdown
Member

jklymak commented Oct 6, 2018

pyplot signatures need more tests

@jklymak jklymak mentioned this pull request Oct 6, 2018
Closed
@jklymak jklymak merged commit a054ee1 into matplotlib:master Oct 6, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 6, 2018
@jklymak
Backport PR matplotlib#12293: Make pyplot more tolerant wrt. 3rd-part…
e4d46a0 
…y subclasses.
@meeseeksmachine meeseeksmachine mentioned this pull request Oct 6, 2018
Merged
@efiring efiring mentioned this pull request Oct 7, 2018
Merged
6 tasks
@anntzer anntzer deleted the tolerant-pyplot branch October 7, 2018 02:02
jklymak added a commit that referenced this pull request Oct 7, 2018
@jklymak
Merge pull request #12426 from meeseeksmachine/auto-backport-of-pr-12…
e3909ac 
…293-on-v3.0.x

Backport PR #12293 on branch v3.0.x (Make pyplot more tolerant wrt. 3rd-party subclasses.)
This was referenced Oct 10, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic approved these changes

@jklymak jklymak jklymak left review comments

@dstansby dstansby dstansby left review comments

Assignees

No one assigned

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Milestone

v3.0.1

Development

Successfully merging this pull request may close these issues.

5 participants

@anntzer @jklymak @QuLogic @dstansby @tacaswell