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

Matplotlib 2.0 compatibility #1616

Merged
merged 25 commits into from Apr 5, 2017

Conversation

@QuLogic
Member

QuLogic commented Dec 7, 2016

Now that matplotlib 2.0.0rc1 is out, I've tested it with ObsPy. Mostly the differences are in text location and small shift in the legend location. There are a couple of larger changes that I am still investigating. I think most of them are not a big deal though. Here is a histogram of RMS/tolerance for the plots:
figure_1
most are <10 times, but some are way off. Not sure why that is yet.

For now, I'm just pushing this to make sure I haven't broken older Matplotlib, but I'll add proper compatibility stuff after. Should I update the figures for 2.0 or just up the tolerance?

Tentatively 1.1.0, but we can bump it if it's holding things up; I think it'll be small enough to go into 1.1.1 if it has to (and we aren't replacing all the images.)

@QuLogic QuLogic added the .imaging label Dec 7, 2016

@QuLogic QuLogic added this to the 1.1.0 milestone Dec 7, 2016

@QuLogic QuLogic changed the title from Matplotlib 2.0 compatibility to [WIP] Matplotlib 2.0 compatibility Dec 7, 2016

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Dec 7, 2016

Member

This looks pretty good. Should we try to get this into 1.0.3 to make it matplotlib 2 testable?

There appear to be some issues with the default style on matplotlib 1.4: http://tests.obspy.org/63097/

Member

krischer commented Dec 7, 2016

This looks pretty good. Should we try to get this into 1.0.3 to make it matplotlib 2 testable?

There appear to be some issues with the default style on matplotlib 1.4: http://tests.obspy.org/63097/

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Dec 8, 2016

Member

Turns out there is no default stylesheet in 1.4, so by default, it's just not going to set one. There's matplotlib/matplotlib#7587 to work out (the first figure is xcorr_pick_corr and the second is the response plots) and then we'll need to regenerate images or increase tolerance for 2.0 and this'll be done.

Member

QuLogic commented Dec 8, 2016

Turns out there is no default stylesheet in 1.4, so by default, it's just not going to set one. There's matplotlib/matplotlib#7587 to work out (the first figure is xcorr_pick_corr and the second is the response plots) and then we'll need to regenerate images or increase tolerance for 2.0 and this'll be done.

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Dec 8, 2016

Member

One more change to ObspyAutoDateFormatter because Matplotlib 2.0 adds more entries to the lookup with full precision. There are still a few differences with date formatting, but I'm not sure how they're triggered. Maybe @megies knows more about this.

Member

QuLogic commented Dec 8, 2016

One more change to ObspyAutoDateFormatter because Matplotlib 2.0 adds more entries to the lookup with full precision. There are still a few differences with date formatting, but I'm not sure how they're triggered. Maybe @megies knows more about this.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Dec 10, 2016

Member

There are still a few differences with date formatting, but I'm not sure how they're triggered. Maybe @megies knows more about this.

@QuLogic, are you're referring to e.g. http://tests.obspy.org/63601/#1? The tick formatter function gets passed the index of the tick by matplotlib (starting with 0 for the left-most x tick), but I assume you've seen that. But.. IIRC matplotlib sadly is not very exact in figuring out what the left-most tick really is, and I think I remember that sometimes matplotlib passes on a non-zero value for the left-most tick (wrongly assuming that there is another tick left of it shown in the figure, which isn't). No idea if that detection routine or the API changed for 2.0.0..

(@barsch any changes to the test report webpage.. somehow the image comparison with the slider isn't there anymore..)

Member

megies commented Dec 10, 2016

There are still a few differences with date formatting, but I'm not sure how they're triggered. Maybe @megies knows more about this.

@QuLogic, are you're referring to e.g. http://tests.obspy.org/63601/#1? The tick formatter function gets passed the index of the tick by matplotlib (starting with 0 for the left-most x tick), but I assume you've seen that. But.. IIRC matplotlib sadly is not very exact in figuring out what the left-most tick really is, and I think I remember that sometimes matplotlib passes on a non-zero value for the left-most tick (wrongly assuming that there is another tick left of it shown in the figure, which isn't). No idea if that detection routine or the API changed for 2.0.0..

(@barsch any changes to the test report webpage.. somehow the image comparison with the slider isn't there anymore..)

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Dec 10, 2016

Member

no changes from my side - will look into tomorrow ...

Member

barsch commented Dec 10, 2016

no changes from my side - will look into tomorrow ...

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Dec 11, 2016

Member

@megies Oddly enough, after a refresh, the images do appear for me (sometimes).

@barsch Also, the images show up over http, but not https; Firefox doesn't like the mixed content because the imgur links are not secure.

Member

QuLogic commented Dec 11, 2016

@megies Oddly enough, after a refresh, the images do appear for me (sometimes).

@barsch Also, the images show up over http, but not https; Firefox doesn't like the mixed content because the imgur links are not secure.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Dec 11, 2016

Member

Indeed, sometimes images show up after a couple of refreshes..

@QuLogic, to debug, you could print pos and the returned tick label here: https://github.com/obspy/obspy/blob/master/obspy/imaging/util.py#L124-L145

Member

megies commented Dec 11, 2016

Indeed, sometimes images show up after a couple of refreshes..

@QuLogic, to debug, you could print pos and the returned tick label here: https://github.com/obspy/obspy/blob/master/obspy/imaging/util.py#L124-L145

@barsch

This comment has been minimized.

Show comment
Hide comment
@barsch

barsch Dec 12, 2016

Member

@megies: mixing http/https was the issue (thx @QuLogic) - should be fixed now

Member

barsch commented Dec 12, 2016

@megies: mixing http/https was the issue (thx @QuLogic) - should be fixed now

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 10, 2017

Member

New Debian 9 "stretch" was frozen some days ago.. and it's already shipping matplotlib 2.0.0.. O_0

Really surprised that they included it so fast and so shortly before the freeze..

Member

megies commented Feb 10, 2017

New Debian 9 "stretch" was frozen some days ago.. and it's already shipping matplotlib 2.0.0.. O_0

Really surprised that they included it so fast and so shortly before the freeze..

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Feb 11, 2017

Member

Yes, Debian was ready to go with Matplotlib 2.0 since the release candidates.

This is currently mostly working except for the change in text location. I would like to see what effect matplotlib/matplotlib#7970 has before regenerating all the images.

Additionally, there's a change in the Cartopy images because it's not fully Matplotlib 2.0 compatible. See SciTools/cartopy#773 for that.

Member

QuLogic commented Feb 11, 2017

Yes, Debian was ready to go with Matplotlib 2.0 since the release candidates.

This is currently mostly working except for the change in text location. I would like to see what effect matplotlib/matplotlib#7970 has before regenerating all the images.

Additionally, there's a change in the Cartopy images because it's not fully Matplotlib 2.0 compatible. See SciTools/cartopy#773 for that.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Feb 23, 2017

Member

matplotlib/matplotlib#8081 seems to be merged know and it should help a lot.

@QuLogic Any ETA on the release of 2.0.1?

Member

krischer commented Feb 23, 2017

matplotlib/matplotlib#8081 seems to be merged know and it should help a lot.

@QuLogic Any ETA on the release of 2.0.1?

@krischer krischer referenced this pull request Feb 23, 2017

Closed

Release 1.0.3 #1561

12 of 16 tasks complete
@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Feb 25, 2017

Member

Possibly this weekend, but actually, I'm not sure that fix will save us from regenerating quite a few images (or upping tolerances.) The trouble is the alignment is the same as before but there's so much other text that's also moved. I think part of the reason is because our images were generated in such older mpl.

Member

QuLogic commented Feb 25, 2017

Possibly this weekend, but actually, I'm not sure that fix will save us from regenerating quite a few images (or upping tolerances.) The trouble is the alignment is the same as before but there's so much other text that's also moved. I think part of the reason is because our images were generated in such older mpl.

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Feb 25, 2017

Member

Oh, I just realized I've been ignoring the tol*=17 for 1.5.x line; if I extend that to 2.0, I go from 81 failures to just 1, even sticking with 2.0.0 (which doesn't have the baseline change.) Most of the changes are the correction of the text baseline that occurred in 1.5.x.

When do we want to regen the images? When do we want to drop support for older mpl?

Member

QuLogic commented Feb 25, 2017

Oh, I just realized I've been ignoring the tol*=17 for 1.5.x line; if I extend that to 2.0, I go from 81 failures to just 1, even sticking with 2.0.0 (which doesn't have the baseline change.) Most of the changes are the correction of the text baseline that occurred in 1.5.x.

When do we want to regen the images? When do we want to drop support for older mpl?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 25, 2017

Member

When do we want to regen the images? When do we want to drop support for older mpl?

We should be pragmatic, so we do what is less work for us. I haven't been able to keep up with this PR about mpl 2 compatibility but if it makes live easier for us to regenerate images now, I'm totally for it.

If at least image size is the same as with current baseline images we can regenerate with 2.0 and use high tolerances for testruns with mpl 1.x.

Member

megies commented Feb 25, 2017

When do we want to regen the images? When do we want to drop support for older mpl?

We should be pragmatic, so we do what is less work for us. I haven't been able to keep up with this PR about mpl 2 compatibility but if it makes live easier for us to regenerate images now, I'm totally for it.

If at least image size is the same as with current baseline images we can regenerate with 2.0 and use high tolerances for testruns with mpl 1.x.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 1, 2017

Member

If at least image size is the same as with current baseline images we can regenerate with 2.0 and use high tolerances for testruns with mpl 1.x.

👍 On this. Let's regenerate them once, once 2.0.1 is out and up the tolerances for the 1.x line. We cannot easily drop support for old matplotlib versions if we want to keep our policy of supporting the packaged versions of all major linux distributions.

Member

krischer commented Mar 1, 2017

If at least image size is the same as with current baseline images we can regenerate with 2.0 and use high tolerances for testruns with mpl 1.x.

👍 On this. Let's regenerate them once, once 2.0.1 is out and up the tolerances for the 1.x line. We cannot easily drop support for old matplotlib versions if we want to keep our policy of supporting the packaged versions of all major linux distributions.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 28, 2017

Member

This is currently mostly working except for the change in text location. I would like to see what effect matplotlib/matplotlib#7970 has before regenerating all the images.

@QuLogic, seems it's merged: matplotlib/matplotlib#8081

@QuLogic since 2.0.1 doesn't seem to have a fixed schedule (?), how about sticking with old baseline images for 1.1.0 and upping tolerances for mpl >=2.0 right now?

Member

megies commented Mar 28, 2017

This is currently mostly working except for the change in text location. I would like to see what effect matplotlib/matplotlib#7970 has before regenerating all the images.

@QuLogic, seems it's merged: matplotlib/matplotlib#8081

@QuLogic since 2.0.1 doesn't seem to have a fixed schedule (?), how about sticking with old baseline images for 1.1.0 and upping tolerances for mpl >=2.0 right now?

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 31, 2017

Member

I regenerated all test images with today's tip of matplotlib's v2.0.x branch (the master branch had some weird stuff going on and didn't work for all plots). I think this is a pretty future proof way of doing things. Locally it works for mpl 1.5 and 2.0. Note that the tolerances for 2.0.0 are quite high due to the aforementioned issues with the tick placement. For the upcoming 2.0.1 they will be much lower.

It kind of sucks that we have to eat the additional file size of the new test images but I think this is a sane way of doing it - and we really need mpl 2.0 support - this gets a bit annoying in daily use.

There is a weird test failure for obspy/signal/tests/test_spectral_estimation.py - test_ppsd_restricted_stacks on matplotlib 1.5.3. @megies Can you have a look?

@QuLogic Is this a sensible thing to do? Does mpl expect a lot of changes to the default style?

Don't merge yet - needs some squashing!

Member

krischer commented Mar 31, 2017

I regenerated all test images with today's tip of matplotlib's v2.0.x branch (the master branch had some weird stuff going on and didn't work for all plots). I think this is a pretty future proof way of doing things. Locally it works for mpl 1.5 and 2.0. Note that the tolerances for 2.0.0 are quite high due to the aforementioned issues with the tick placement. For the upcoming 2.0.1 they will be much lower.

It kind of sucks that we have to eat the additional file size of the new test images but I think this is a sane way of doing it - and we really need mpl 2.0 support - this gets a bit annoying in daily use.

There is a weird test failure for obspy/signal/tests/test_spectral_estimation.py - test_ppsd_restricted_stacks on matplotlib 1.5.3. @megies Can you have a look?

@QuLogic Is this a sensible thing to do? Does mpl expect a lot of changes to the default style?

Don't merge yet - needs some squashing!

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 31, 2017

Member

and we really need mpl 2.0 support

totally 👍

Member

megies commented Mar 31, 2017

and we really need mpl 2.0 support

totally 👍

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Apr 1, 2017

Member

Amended the last commit and force-pushed to trigger Travis.

Member

megies commented Apr 1, 2017

Amended the last commit and force-pushed to trigger Travis.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Apr 1, 2017

Member

There is a weird test failure for obspy/signal/tests/test_spectral_estimation.py - test_ppsd_restricted_stacks on matplotlib 1.5.3. @megies Can you have a look?

Did you see this comment? It might have gotten overlooked in my long comment.

Member

krischer commented Apr 1, 2017

There is a weird test failure for obspy/signal/tests/test_spectral_estimation.py - test_ppsd_restricted_stacks on matplotlib 1.5.3. @megies Can you have a look?

Did you see this comment? It might have gotten overlooked in my long comment.

Use DejaVu Sans on mpl2.
It's nearly the same as Bitstream Vera Sans, but guaranteed to exist on
Matplotlib 2+ in the same way.
@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Apr 4, 2017

Member

Appveyor fails because I accidentially pushed to the obspy fork again...sorry about that...

Member

krischer commented Apr 4, 2017

Appveyor fails because I accidentially pushed to the obspy fork again...sorry about that...

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Apr 4, 2017

Member

Rebased on latest master and to remove the duplicate test images generations. Now passes on all travis runners and appveyor :) IMHO ready to merge once the docker test bots are done with it :)

Member

krischer commented Apr 4, 2017

Rebased on latest master and to remove the duplicate test images generations. Now passes on all travis runners and appveyor :) IMHO ready to merge once the docker test bots are done with it :)

@@ -125,10 +136,9 @@ install:
sqlalchemy
mock
nose
gdal

This comment has been minimized.

@megies

megies Apr 5, 2017

Member

gdal is used in io.shapefile tests.. i suppose it was making problems with Py 3.6? maybe we can install it in at least a few Travis runs?

@megies

megies Apr 5, 2017

Member

gdal is used in io.shapefile tests.. i suppose it was making problems with Py 3.6? maybe we can install it in at least a few Travis runs?

This comment has been minimized.

@krischer

krischer Apr 5, 2017

Member

I opened a separate issue to re-enable it (#1749) as I'd really like to get this PR merged.

@krischer

krischer Apr 5, 2017

Member

I opened a separate issue to re-enable it (#1749) as I'd really like to get this PR merged.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Apr 5, 2017

Member

Works for everything :) Someone please review and merge!

Member

krischer commented Apr 5, 2017

Works for everything :) Someone please review and merge!

@megies

megies approved these changes Apr 5, 2017

Looking good, thanks @QuLogic and @krischer!

@megies megies merged commit 8ab00e8 into obspy:master Apr 5, 2017

7 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 93.44% of diff hit (target 90%)
Details
codecov/project 87.37% (+1.2%) compared to 45c0bbb
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
docker-deb-buildbot Deb packaging and testing succeeded
Details
docker-testbot Docker tests succeeded
Details

@megies megies deleted the QuLogic:mpl2 branch Apr 5, 2017

@megies megies changed the title from [WIP] Matplotlib 2.0 compatibility to Matplotlib 2.0 compatibility Apr 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment