Skip to content
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

avoid failing doctests on new mpl 2.2 #2089

Merged
merged 10 commits into from Apr 10, 2018

Conversation

Projects
None yet
3 participants
@megies
Copy link
Member

commented Mar 16, 2018

matplotlib changed Figure.__repr__() it seems..

before it was:
<matplotlib.figure.Figure object at 0x...>

now it is:
<Figure size 640x480 with 0 Axes>

see e.g.:
https://ci.appveyor.com/project/obspy/obspy/build/1.0.5829-master/job/vqi79t5r05h7ttn4#L753

@megies megies added the testing label Mar 16, 2018

@megies megies added this to the 1.1.1 milestone Mar 16, 2018

@megies

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2018

No idea where the remaining CI fail is coming from but it looks like it's UTCDateTime related.. any idea @d-chambers or @barsch maybe?

https://ci.appveyor.com/project/obspy/obspy/build/1.0.5832-maintenance_1.1.x/job/5rv6n5rg6b5aq7lf#L695

@megies

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2018

This PR reduces testing issues with new dependency versions to one fail:

 ======================================================================
FAIL: test_multiple_sampling_rates (obspy.imaging.tests.test_scan.ScanTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/megies/git/obspy/obspy/imaging/tests/test_scan.py", line 179, in test_multiple_sampling_rates
    self.assertEqual(expected, out.stdout.splitlines())
AssertionError: Lists differ: [u'XX.TEST..BHZ 2008-01-15T00:... != ['XX.TEST..BHZ 2008-01-15T00:0...

First differing element 0:
u'XX.TEST..BHZ 2008-01-15T00:00:01.000000Z 2008-01-15T00:00:00.899995Z -0.100'
'XX.TEST..BHZ 2008-01-15T00:00:01.000000Z 2008-01-15T00:00:00.900000Z -0.100'

- [u'XX.TEST..BHZ 2008-01-15T00:00:01.000000Z 2008-01-15T00:00:00.899995Z -0.100',
?  -                                                              - ^^^^

+ ['XX.TEST..BHZ 2008-01-15T00:00:01.000000Z 2008-01-15T00:00:00.900000Z -0.100',
?                                                                 ^^^^^

-  u'XX.TEST..BHZ 2008-01-15T00:00:01.899999Z 2008-01-15T00:00:02.000000Z 0.100']
?  -                                  - ^^^^

+  'XX.TEST..BHZ 2008-01-15T00:00:01.900000Z 2008-01-15T00:00:02.000000Z 0.100']
?                                     ^^^^^


----------------------------------------------------------------------

Anybody has an idea about that one?

@d-chambers

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

I am running into this sporadically in a PR I am working on as well. I will dig into it when I get a few minutes within the next few days.

@d-chambers

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

We can blame matplotlib again for this one. This line:

start__, end__ = num2date((start_, end_))

returns different datetime objects for different versions of matplotlib. This code:

from matplotlib.dates import date2num, num2date
num2date(733056.00001041661)

returns

datetime.datetime(2008, 1, 15, 0, 0, 0, 899995)

for matplotlib 2.0.2

but it returns

datetime.datetime(2008, 1, 15, 0, 0, 0, 900000)

for matplotlib 2.2.2

IMHO tests that compare str representations of objects are extremely brittle. It is much better to work with python objects directly and check types, or that values are close, etc etc.

I will see if I can make this test more robust.

@d-chambers

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

If I only check that the datetimes are "close" then the image comparisons fail.
We are expecting this:
image
but getting this:
https://i.imgur.com/p8SPjz0.png
image

@krischer krischer referenced this pull request Mar 26, 2018

Merged

Add more features to nordic IO #1924

10 of 11 tasks complete
@krischer

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

Ah this is silly. Oh well - I think the best way to handle this is to update the image using the latest matplotlib and just use a much higher tolerance for older matplotlib versions. Do you have a branch somewhere where you fixed the other issue?

@d-chambers

This comment has been minimized.

Copy link
Member

commented Mar 27, 2018

Just pushed it here. All it does is parse the captured str output and convert to proper date times in order to use numpy's isclose function.

It seems like the image comparison test often just detect changes rather than verify correctness, but there probably isn't a better (automated) way to do things.

@krischer krischer referenced this pull request Mar 29, 2018

Closed

Station plot not working #2097

@d-chambers

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

I think the best way to handle this is to update the image using the latest matplotlib and just use a much higher tolerance for older matplotlib versions

Ok, I think I did this right. All tests are passing on 3.6/ubuntu, we will see what CI does.

@d-chambers

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

Ok, I am not sure what is going wrong with the tolerances in the image compare. It seems right when I run it locally but is off by a factor of 10 when the CI runs.

@krischer

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

Something went wrong with rebasing/merging here.

Ok, I am not sure what is going wrong with the tolerances in the image compare. It seems right when I run it locally but is off by a factor of 10 when the CI runs.

If the matplotlib + freetype versions locally are the same as in the CI then this should not happen. We also at one point coded our own RMS tolerance method to not be reliant on some outside package which might change it going from one version to the next.

@d-chambers

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

Something went wrong with rebasing/merging here.

Indeed. I merged in master but this branch needs to be based on maintenance_1.1.x. Sorry for screwing this up, I should have caught that before pushing. In order to fix it I created a new branch from maintenance_1.1.x and cherry-picked the commit differences between this branch and master. There is probably a more elegant solution but my git-fu was not sufficient to carry it out. I pushed the new branch here. I think we just need to force push it onto mpl_doctest but I wanted to check with you guys first before doing anything of the sort.

We also at one point coded our own RMS tolerance method to not be reliant on some outside package which might change it going from one version to the next.

I ended up just instantiating an ImageComparison object and manually setting the tol attribute before entering the context manager (here). Is there a better approach? I didn't see a manual RMS option like you mentioned.

@krischer

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

I think we just need to force push it onto mpl_doctest but I wanted to check with you guys first before doing anything of the sort.

Force pushing is fine as long as it does not interfere with anyone else's work. But we are small enough to usually just do it. Also the master and maintenance_xxx branches are protected from force pushes so it should be really hard to screw things up.

I ended up just instantiating an ImageComparison object and manually setting the tol attribute before entering the context manager (here). Is there a better approach? I didn't see a manual RMS option like you mentioned.

The link does not work so I cannot check but I think I usually do it differently. The ImageComparision has a reltol attribute, see e.g. here:

with ImageComparison(self.image_dir, 'network_location-basemap3.png',

This is a relative tolerance as we have some internal logic to set the tolerance based on the matplotlib version:

# Adjust the tolerance based on the matplotlib version. This works

The whole things is pretty whacky and I would love if somebody could cook up a cleaner approach but I don't really see a way to do this. I guess its just the reality of trying to handle a large dependency matrix. The end result is that the images are tested to fairly great accuracy for the most recent matplotlib versions and we make sure the images look at least similar with older versions.

@d-chambers d-chambers force-pushed the mpl_doctests branch from 9202b50 to fbfc890 Apr 3, 2018

@d-chambers

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

I originally tried setting the relative tolerance but there must have been some differences in the versions of the packages it takes into account because the absolute tolerance ended up being quite different between from my dev environment and the CI environment. Rather than mess with the package versions I opted for just setting the tol attribute for this one problematic test. Otherwise, when we update the versions of packages in the CI this test could easily break again.

@d-chambers

This comment has been minimized.

Copy link
Member

commented Apr 5, 2018

Does this look alright to merge?

else:
# datetimes just need to be close
t1, t2 = utc1.timestamp, utc2.timestamp
self.assertTrue(abs(t1 - t2) < .001)

This comment has been minimized.

Copy link
@megies

megies Apr 9, 2018

Author Member

I'm a bit puzzled why we have to change this unit test.. there were some PRs with changes to UTCDateTime, are those responsible for needing this change?

This comment has been minimized.

Copy link
@d-chambers

d-chambers Apr 9, 2018

Member

@megies, no I thought that at first too but it is actually due to a change in how matplotlib handles rounding (see this comment). This test needed to change in order to not simply compare the string representation of the matplotlib output (which for some reason is version dependent) but rather to test that the values returned are very close to the expected result.

This comment has been minimized.

Copy link
@megies

megies Apr 10, 2018

Author Member

Oh.. I got it now. Thanks for clarification. So the reason is that the data during obspy-scan goes through a date2num-num2date cycle and our "expected" data is sitting in the middle, created with an older matplotlib than is used in CI.

This also implies that data saved by obspy-scan on older mpl versions gets deserialized with slightly different results when using a newer matplotlib.. :-/

@megies
Copy link
Member Author

left a comment

Good to go!

@megies megies merged commit 9c1f9ce into maintenance_1.1.x Apr 10, 2018

5 of 7 checks passed

codecov/patch 81.48% of diff hit (target 90%)
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 88.08% (+1.18%) compared to aa20235
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@megies megies deleted the mpl_doctests branch Apr 10, 2018

@megies megies referenced this pull request Apr 10, 2018

Open

Maintenance needed on "obspy-scan" #2102

0 of 3 tasks complete

@megies megies referenced this pull request May 7, 2018

Merged

fix map plots for old mpl 1.2 #2139

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.