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

Fix test failures on master #2269

Merged
merged 29 commits into from Dec 13, 2018

Conversation

Projects
None yet
4 participants
@d-chambers
Copy link
Member

d-chambers commented Nov 24, 2018

What does this PR do?

Fixes the test failures on master due to flake8, escape characters in python 3.7, and a few other misc. issues.

Why was it initiated? Any relevant Issues?

The test suite on master was failing due to several small issues.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+ DOCS"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .

@d-chambers d-chambers added this to the 1.2.0 milestone Nov 24, 2018

@d-chambers

This comment has been minimized.

Copy link
Member Author

d-chambers commented Nov 24, 2018

Well it works on my machine 😉

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

calum-chamberlain commented Dec 3, 2018

+TESTS:ALL

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

calum-chamberlain commented Dec 3, 2018

I have done a few things in here to make some failing network tests pass - these all seemed to be due to changes in sensor availability, or changes in how the service was delivered.

I haven't fixed a failing fdsn test obspy.clients.fdsn.tests.ClientTestCase.test_dataselect_bulk which checks for a few things, including the use of the minimumlength argument. To me it looks like how IRIS handle that might have changed. The test specifies a start and end time 4s apart and a minimumlength of 5. We are now getting traces less than minimum length. In the FDSN docs it isn't clear whether starttime and endtime or minimumlength should take priority.

For the molepeople plot issues, I did some experimentation and it looks like proj4=5.2.0 is causing the issue - I tested 4.9.3 and 5.1.0 and found both to work. I have added a check of the version when importing obspy.imaging.maps which follows the other warnings. I also pinned the version for ci to 4.9.3, this probably isn't ideal.

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

calum-chamberlain commented Dec 3, 2018

I'm not sure what is going on with the appveyor fail relating to the use of TemporaryWorkingDirectory - previously I added a check to make sure that the files were closed (they are) but it still complains that something else is accessing the files. Not sure if we should just ignore it?

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

calum-chamberlain commented Dec 4, 2018

The axis._hold type issue is related to this I think. Added a skipIf for matplotlib version 3.0.1 and basemap >= 1.1.0.
I can't reproduce the appveyor test_simulate fail, but it looks like it might be similar to #2188. I couldn't reproduce on my 64Bit windows with a clean conda env using the same packages as appveyor.

@d-chambers

This comment has been minimized.

Copy link
Member Author

d-chambers commented Dec 5, 2018

I would still suggest we merge this as soon as possible

Good idea. I wont have time to do anything on this for a week or two so go ahead and merge whenever you are happy @calum-chamberlain. It is nice to see Travis green again.

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

calum-chamberlain commented Dec 5, 2018

So the only thing that is failing now is some stream comparison in appveyor. But I can't reproduce, and it seems to be transient on appveyor - sometimes it passes sometimes it doesn't. Not sure what is going on...

We could just check allclose on data on windows as was done for #2188?

I have done some questionable things in terms of skipping tests for specific versions that either do not work (such as basemap with mpl 3.0.1, and skipping the PPSD plot test for mpl>=3 which moves the xtick locations - I think the new locations are nicer and result in a better use of the figure space). And ignoring the network test change for minimumlength.

I'm also not super keen on the change I made to taup - changing from resizing inplace to generating a new array with np.resize as a function might have implications for memory and speed, but I don't know the taup stuff well enough to know if the size of these arrays is going to be an issue.

Otherwise, ready for review and merge IMHO.

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 6, 2018

We could just check allclose on data on windows as was done for #2188?

This the only info we have on the fail?

Traceback (most recent call last):
  File "C:\projects\obspy\obspy\core\tests\test_stream.py", line 2207, in test_simulate_seedresp_parser
    self.assertEqual(tr1, tr2)
AssertionError: <obspy.core.trace.Trace object at 0x00000002A2D67080> != <obspy.core.trace.Trace object at 0x00000002A66E6438>

If that's the case, we should add an assertEqual on the tr.data to see how big the difference is, first of all?

skipping tests for specific versions that either do not work (such as basemap with mpl 3.0.1,

this is the right thing to do, IMHO. we won't see a lot of mpl 3.0.1 anyway once the next minor release is out (that'll fix this hold issue)

skipping the PPSD plot test for mpl>=3 which moves the xtick locations - I think the new locations are nicer and result in a better use of the figure space

At some point we want to update our baseline images to mpl 3. Maybe we want to wait for mpl 3.0.2, though. We can then have higher tolerances for failing tests with mpl 2.x
I'm okay with merging this right now, just open an issue please as a reminder to switch baseline images to mpl 3 at some point, ideally against the 1.2.0 milestone?

And ignoring the network test change for minimumlength.

Hmm. not sure about this one.. maybe better leave it in as a failing test? I strongly believe it's a server side bug that will be fixed pretty fast by IRIS once it gets the attention of the person in charge (I dropped Chad a mail to forward the issue but he didn't respond yet, probably busy with AGU preps).

I'm also not super keen on the change I made to taup - changing from resizing inplace to generating a new array with np.resize as a function might have implications for memory and speed, but I don't know the taup stuff well enough to know if the size of these arrays is going to be an issue

This looks like we need to be careful not to break things.. dunno. I also don't know the taup stuff much, but this looks like it can break references to the same data array?

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

calum-chamberlain commented Dec 6, 2018

Thanks for those comments @megies.

  • I have removed the changes to obspy.taup.tau_branch, so I expect those doctests to fail. I think the fails could be related to how the doctests are run - maybe each doctest is trying to work on the same array at the same time?
  • Removed the expected fail flag on the test with the minimumlength issue.
  • Added some (hopefully useful) extra comparisons for the two simulate tests that seemed flakey, hopefully we can see whether there really is an issue or not with that.
@chad-iris

This comment has been minimized.

Copy link
Contributor

chad-iris commented Dec 7, 2018

It fails at the moment because minimumlength looks like it is being ignored. I'm not sure if IRIS changed how they respond to requests or if it is on our end?

Indeed the option was effectively being ignored. This is now fixed at IRIS.

Note: We are deprecating this option as we have a much better way to do this with our Research Ready Data Sets service, which allows selection of data based on quality metrics including time coverage. It's supported for now, but not promised to be working in the future.

It's been broken for 6 months and this is the first we've heard about it, which aligns with my suspicion that it's not often used.

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

calum-chamberlain commented Dec 7, 2018

@chad-iris thanks! Personally I haven't used the minimumlength constraint, and wouldn't have known it wasn't working if not for the tests. Nice to hear about the RRDS too, I wasn't aware of that.
Sounds like it might be a good idea for us to not base a test on this then if it won't be supported in the future.

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

calum-chamberlain commented Dec 7, 2018

The simulate fail that remains on windows passes the tests comparing stats and using allclose (with default settings, so rtol=1e-7) but still fails the stream and trace comparison tests

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 11, 2018

Sounds like it might be a good idea for us to not base a test on this then if it won't be supported in the future.

Without looking at the (test) code, we could replace the test by checking that the appropriate URL is built inside the client. But then again, I am confident that the URL building inside the client is tested thoroughly enough, so probably we can just get rid of that parameter when downloading data in tests. On the other hand, since it's fixed on server side, we can also just leave it as is for now.

EDIT: restarted Travis, so that minimumlength fail should go away then

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 11, 2018

The simulate fail that remains on windows passes the tests comparing stats and using allclose (with default settings, so rtol=1e-7) but still fails the stream and trace comparison tests

I think we should either..

  • subclass unittest.TestCase and implement a proper error message when comparing Stream and Trace objects (but I think @krischer was against that when I brought it up a looong time ago)
  • implement our own utility assertion function to do that comparison and not use unittest.TestCase.assertEqual() on streams and traces (maybe with a rtol kwarg that gets passed to numpy), or
  • replace that short and sweet self.assertEqual(...) that fails on slight numerical differences in the arrays by a more clunky separate comparison of stats and allclose on the data
@d-chambers

This comment has been minimized.

Copy link
Member Author

d-chambers commented Dec 11, 2018

I prefer the second option. It should also have an option to ignore processing attribute of the stats object, as that has been a sticking point in the past as well. Although this would be a nice feature, it probably belongs in a separate PR no?

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

calum-chamberlain commented Dec 12, 2018

The way I thought of doing this was manipulating the Trace.__eq__ function to give it an rtol kwarg, and maybe a processing kwarg as well, then we can let people in user-space use the functions as well...? Then we could do an assertTrue(stream_1.__eq__(stream_2, rtol=2, processing=False)) type thing.

It is not that pretty adding extra arguments to dunder functions tough, so I'm in favour of option 2 and adding it to the core.util.testing module. But agree, separate PR would be good for that.

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 13, 2018

OK sounds good, so probably add a utility Stream/Trace assertion function in core.util.testing but postpone for a future PR.

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 13, 2018

In any case, I'm gonna merge this now to not have it in limbo for too long. Thanks for the initiative on this tedious matter @d-chambers and @calum-chamberlain.

@megies megies merged commit 118347c into master Dec 13, 2018

1 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details

@megies megies deleted the fix_flake_8 branch Dec 13, 2018

@d-chambers d-chambers referenced this pull request Jan 9, 2019

Merged

Fix taup py37 failures #2280

8 of 9 tasks complete

@d-chambers d-chambers referenced this pull request Jan 21, 2019

Open

Steam/Trace almost_equal method #2286

9 of 9 tasks complete
@megies

This comment has been minimized.

Copy link
Member

megies commented Feb 12, 2019

We might decide to backport this for 1.1.1 to get clean CI builds, probably, see #1983

megies added a commit that referenced this pull request Feb 14, 2019

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.