Make pyplot more tolerant wrt. 3rd-party subclasses. by anntzer · Pull Request #12293 · matplotlib/matplotlib · GitHub

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

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)

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.

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

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

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

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

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

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.

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

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

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

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

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

Can you push it?

Done!

Test added

Works for Cartopy.

pyplot signatures need more tests

…y subclasses.

…293-on-v3.0.x Backport PR #12293 on branch v3.0.x (Make pyplot more tolerant wrt. 3rd-party subclasses.)

anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Sep 26, 2018

anntzer added this to the v3.0.x milestone Sep 26, 2018

anntzer force-pushed the tolerant-pyplot branch 2 times, most recently from 0755ea7 to 7ddf5fd Compare September 26, 2018 08:35

jklymak previously requested changes Sep 26, 2018

View reviewed changes

anntzer mentioned this pull request Oct 5, 2018

plt.stackplot() does not work with 3.0.0 #12405

Closed

tacaswell approved these changes Oct 5, 2018

View reviewed changes

anntzer force-pushed the tolerant-pyplot branch from 7ddf5fd to 7809b91 Compare October 6, 2018 10:21

dstansby previously requested changes Oct 6, 2018

View reviewed changes

Add small stackplot smoke test

f6ba2b7

QuLogic approved these changes Oct 6, 2018

View reviewed changes

jklymak mentioned this pull request Oct 6, 2018

pyplot signatures need more tests #12425

Closed

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

Backport PR matplotlib#12293 : Make pyplot more tolerant wrt. 3rd-part…

e4d46a0

…y subclasses.

meeseeksmachine mentioned this pull request Oct 6, 2018

Backport PR #12293 on branch v3.0.x (Make pyplot more tolerant wrt. 3rd-party subclasses.) #12426

Merged

efiring mentioned this pull request Oct 7, 2018

Scatter color: moving #10809 forward #12422

Merged

6 tasks

anntzer deleted the tolerant-pyplot branch October 7, 2018 02:02

jklymak added a commit that referenced this pull request Oct 7, 2018

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

New function signatures in pyplot break Cartopy #12288

Closed

pyplot.step broken in 3.0.0? #12515

Closed

Make pyplot more tolerant wrt. 3rd-party subclasses. by anntzer · Pull Request #12293 · matplotlib/matplotlib · GitHub

Repository navigation

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

Make pyplot more tolerant wrt. 3rd-party subclasses.#12293jklymak merged 2 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom anntzer:tolerant-pyplotanntzer/matplotlib:tolerant-pyplotCopy head branch name to clipboard

Conversation

anntzer commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commented Sep 26, 2018

Uh oh!

jklymak commented Sep 26, 2018

Uh oh!

anntzer commented Sep 26, 2018

Uh oh!

jklymak commented Sep 27, 2018

Uh oh!

QuLogic commented Sep 27, 2018

Uh oh!

anntzer commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jklymak commented Sep 28, 2018

Uh oh!

anntzer commented Sep 29, 2018

Uh oh!

anntzer commented Oct 6, 2018

Uh oh!

dstansby left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commented Oct 6, 2018

Uh oh!

dstansby commented Oct 6, 2018

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commented Oct 6, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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

anntzer commented Sep 26, 2018 •
edited
Loading

anntzer commented Sep 27, 2018 •
edited
Loading