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

Add more add_location tests #1000

Merged
merged 9 commits into from
Mar 21, 2023
Merged

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented Mar 20, 2023

Addresses all but one of the tests specified in #769.

test_consolidate_integration.py::test_add_location now includes two additional sample raw files, including an AZFP file. The test files now encompass:

  • EK60 with Platform lat-lon data (the file previously tested)
  • EK60 with no lat-lon data (from OOI)
  • AZFP with no lat-lon data, where the test uses update_location to first add a fixed-location single point

The tests in this PR currently include:

  • Previously existing, but now run also on an AZFP file:
    • Presence of latitude and longitude variables
    • Absence of time variables
    • Running the above set of tests on nmea_sentence="GGA" for the EK60 dataset with Platform lat-lon data
  • New tests:
    • When source echodata object has no valid lat-lon data, test for expected ValueError
    • No longitude or latitude value is null
    • longitude and latitude variables have a single dimension, ping_time
    • (Added in latest commit) Interpolated lat/lon values added by add_latlon function match up with values interpolated in the test
    • (Added in latest commit) For single-point case, lat-lon coordinates each contains a single unique value that matches the value passed to update_platform

Remaining test: Checking the added (interpolated) lat/lon values between the add_latlon function and directly computed from scipy.interpolate

Also, see #769 (comment) for findings about time1 vs time3 dimensions associated with water_level, and for the outcome of the first interpolated latitude and longitude value being nan. In this PR I modified add_location to set the first value of interpolated latitude and longitude equal to the second value use the scipy.interpolate.interp1d fill_value="extrapolate" scheme. Let's discuss this.

…& lon values that were being interpolated to nan
@emiliom emiliom added the tests label Mar 20, 2023
@emiliom emiliom added this to the 0.7.0 milestone Mar 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2023

Codecov Report

Merging #1000 (5b0e1c0) into dev (b802e8c) will increase coverage by 12.47%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##              dev    #1000       +/-   ##
===========================================
+ Coverage   79.63%   92.10%   +12.47%     
===========================================
  Files          66        3       -63     
  Lines        5745      152     -5593     
===========================================
- Hits         4575      140     -4435     
+ Misses       1170       12     -1158     
Flag Coverage Δ
unittests 92.10% <100.00%> (+12.47%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
echopype/consolidate/api.py 94.52% <100.00%> (+2.73%) ⬆️

... and 63 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@emiliom
Copy link
Collaborator Author

emiliom commented Mar 21, 2023

Ok, done. I decided to use xarray interpolate in the test rather than scipy.interpolate, for expedience. I don't think we gain much by using scipy.interpolate; or at least, I don't think it's worth the extra effort right now.

@emiliom emiliom self-assigned this Mar 21, 2023
Comment on lines 287 to 288
assert not ds_test["longitude"].isnull().any()
assert not ds_test["latitude"].isnull().any()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These will have to change if we do not extrapolate the first position.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed these tests. Now that nan lon & lat values may be valid, I can't think of a general, useful test involving nan's. But I think the other tests already cover all ground: lon & lat contain the ping_time dimension (and no other dim), and the values match against interpolated values (that may include nan's) or fixed-point broadcasting. BTW, I modified the comparison of interpolated values to account for nan's.

Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emiliom : I am done reviewing this. My comments are mostly small and some completely textual. The only exception is the one involving extrapolation. Let me know what you think on that.

Co-authored-by: Wu-Jung Lee <leewujung@gmail.com>
@emiliom
Copy link
Collaborator Author

emiliom commented Mar 21, 2023

Thanks! I'll remove the extrapolation and update the tests to reflect that change.

@emiliom emiliom requested a review from leewujung March 21, 2023 18:19
Copy link
Member

@leewujung leewujung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks @emiliom. Please feel free to merge this!

@emiliom emiliom merged commit cd233fb into OSOceanAcoustics:dev Mar 21, 2023
@emiliom emiliom deleted the add_latlon-tests branch March 21, 2023 21:14
@emiliom emiliom changed the title Add more add_location tests and weak add_location Add more add_location tests Mar 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants