Has Pytest style unit tests
Code isFlake 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 added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label

Sep 26, 2018

anntzer added this to thev3.0.x milestone

Sep 26, 2018

anntzer force-pushed thetolerant-pyplot branch 2 times, most recently from0755ea7 to7ddf5fdCompare

September 26, 2018 08:35

jklymak previously requested changes

Sep 26, 2018

View reviewed changes

Copy link

Member

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)

Copy link

ContributorAuthor

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

Copy link

Member

jklymak commentedSep 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

Copy link

ContributorAuthor

anntzer commentedSep 26, 2018

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

Copy link

Member

jklymak commentedSep 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

Copy link

Member

QuLogic commentedSep 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 thepyplot API, just the following:

$ rg --no-filename -o 'plt\.[A-Za-z0-9_]+' | sort -uplt.autoscaleplt.axesplt.clfplt.closeplt.contourplt.contourfplt.drawplt.figureplt.gcaplt.gcfplt.get_backendplt.imreadplt.imshowplt.pcolorplt.pcolormeshplt.plotplt.savefigplt.scatterplt.showplt.subplotplt.subplots_adjustplt.switch_backendplt.xlimplt.ylim

Copy link

ContributorAuthor

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

Copy link

Member

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

Copy link

ContributorAuthor

anntzer commentedSep 29, 2018

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

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

jklymak dismissed theirstale 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.

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 otherwisewe generate a call of the form `foo=foo, *args` which is incorrectlyunderstood as `*args, foo=foo`.

anntzer force-pushed thetolerant-pyplot branch from7ddf5fd to7809b91Compare

October 6, 2018 10:21

Copy link

ContributorAuthor

anntzer commentedOct 6, 2018

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

dstansby previously requested changes

Oct 6, 2018

View reviewed changes

Copy link

Member

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

Copy link

ContributorAuthor

anntzer commentedOct 6, 2018

Can you push it?

Add small stackplot smoke test

f6ba2b7

Copy link

Member

dstansby commentedOct 6, 2018

Done!

dstansby dismissed theirstale review

October 6, 2018 15:31

Test added

QuLogic approved these changes

Oct 6, 2018

View reviewed changes

Copy link

Member

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.

Copy link

Member

jklymak commentedOct 6, 2018

pyplot signatures need more tests

jklymak mentioned this pull request

Oct 6, 2018

pyplot signatures need more tests#12425

Closed

jklymak merged commita054ee1 intomatplotlib:master

Oct 6, 2018

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request

Oct 6, 2018

Backport PRmatplotlib#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#12426from meeseeksmachine/auto-backport-of-pr-12…

e3909ac

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

This was referencedOct 10, 2018

New function signatures in pyplot break Cartopy#12288

Closed

pyplot.step broken in 3.0.0?#12515

Closed

Labels

Release critical

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

5 participants

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

anntzer commentedSep 26, 2018• editedLoading 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 commentedSep 26, 2018

Uh oh!

jklymak commentedSep 26, 2018

Uh oh!

anntzer commentedSep 26, 2018

Uh oh!

jklymak commentedSep 27, 2018

Uh oh!

QuLogic commentedSep 27, 2018

Uh oh!

anntzer commentedSep 27, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

jklymak commentedSep 28, 2018

Uh oh!

anntzer commentedSep 29, 2018

Uh oh!

anntzer commentedOct 6, 2018

Uh oh!

dstansby left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commentedOct 6, 2018

Uh oh!

dstansby commentedOct 6, 2018

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commentedOct 6, 2018

Uh oh!

Uh oh!

anntzer commentedSep 26, 2018•
edited
Loading

anntzer commentedSep 27, 2018•
edited
Loading