OSOceanAcoustics / echopype

Enabling interoperability and scalability in ocean sonar data analysis
https://echopype.readthedocs.io/
Apache License 2.0
94 stars 73 forks source link

`parse_azfp` now parses AZFP pressure data according to given matlab code #1189

Closed praneethratna closed 10 months ago

praneethratna commented 10 months ago

Addresses #1181 and now parse_azfp.py parses AZFP pressure data according to Matlab file shared by Steve.

CC @leewujung

codecov-commenter commented 10 months ago

Codecov Report

Merging #1189 (9514ae5) into dev (0bc534e) will decrease coverage by 9.20%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1189      +/-   ##
==========================================
- Coverage   77.04%   67.85%   -9.20%     
==========================================
  Files          67       19      -48     
  Lines        5908     3070    -2838     
==========================================
- Hits         4552     2083    -2469     
+ Misses       1356      987     -369     
Flag Coverage Δ
unittests 67.85% <100.00%> (-9.20%) :arrow_down:

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

Files Coverage Δ
echopype/convert/parse_azfp.py 94.28% <100.00%> (+0.75%) :arrow_up:
echopype/convert/set_groups_azfp.py 97.95% <ø> (ø)

... and 50 files with indirect coverage changes

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

praneethratna commented 10 months ago

@emiliom I have added the fix for failing CI tests here. It is caused due to charset-normalizer dependency version error for which version should be explicitly set to 3.1.0.

Edit: Fix for failing CI tests are removed from this PR and added in #1191.

emiliom commented 10 months ago

@praneethratna I've reviewed this PR and it looks great. See my comments about a spelling mistake and small, cosmetic changes.

Let's wait until we get an AZFP data file that does have pressure data, then you can add a test.

The open question then will be whether to add to this PR the parts that create and populate variables in set_groups_azfp.py, including pressure and vertical_offset; or do that in a separate PR. Adding it to this PR is seems fine.

praneethratna commented 10 months ago

The open question then will be whether to add to this PR the parts that create and populate variables in set_groups_azfp.py, including pressure and vertical_offset; or do that in a separate PR. Adding it to this PR is seems fine.

@emiliom I have added the changes for including pressure attribute into Environment group in this PR. I think the changes for 2nd point that you've mentioned here can be added in a separate PR.

emiliom commented 10 months ago

@praneethratna everything looks good! The new test looks good. But I'd like to run this locally before approving the PR. I'll do that later, hopefully today.

I do have a couple of suggestions, though, to improve the tests in test_convert_azfp.py that are related to pressure:

praneethratna commented 10 months ago

I do have a couple of suggestions, though, to improve the tests in test_convert_azfp.py that are related to pressure:

  • Assuming the data file contains temperature too, let's expand the scope of your new test to do an analogous test for temperature. That should be pretty easy to add. Then rename the test to def test_convert_azfp_01a_pressure_temperature, and update the docstring.
  • Modify test_convert_azfp_01a_notemperature_notilt to test for the pressure variable being present and all-nan, just like temperature. Literally, copy and adapt the two lines from the temperature test! The new test for pressure will be run on the same data file being used there. Then rename the test to, say, test_convert_azfp_01a_no_temperature_pressure_tilt

@emiliom The suggestions seems relevant and beneficial for testing the new pressure attribute. I pushed the changes for suggestions that you have mentioned in the latest commit!

emiliom commented 10 months ago

Thanks! Your changes look good. I made an inline commit that just updated a test docstring.

Once the CI completes, assuming they're successful, I'll go ahead and approve and merge the PR.