OSOceanAcoustics / echopype

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

Set EK60/80 `Platform` and `NMEA` nan timestamp to first `ping_time` value [all tests ci] #1154

Closed emiliom closed 1 year ago

emiliom commented 1 year ago

Addresses the first two xarray warnings identified in #1153. Implements the solution described there, for EK60.

Setting single-valued nan Platform.time1 cases to the first ping_time value is consistent with the approach currently used with AZFP. EK80 may benefit from this change, too, but I'm not aware of a test case raw file, plus we're short on time.

See discussion in #1153.

[@leewujung edits for future reference]: Saildrone EK80 data by default does not have the lat/lon data since GPS is not plugged into the echosounder itself.

leewujung commented 1 year ago

EK80 may benefit from this change, too, but I'm not aware of a test case raw file, plus we're short on time.

The saildrone EK80 data by default does not have the lat/lon data since GPS is not plugged into the echosounder itself, so those would be the example raw files.

codecov-commenter commented 1 year ago

Codecov Report

Merging #1154 (0649bb4) into dev (5abca87) will increase coverage by 0.02%. The diff coverage is 94.11%.

@@            Coverage Diff             @@
##              dev    #1154      +/-   ##
==========================================
+ Coverage   82.32%   82.35%   +0.02%     
==========================================
  Files          64       64              
  Lines        5789     5803      +14     
==========================================
+ Hits         4766     4779      +13     
- Misses       1023     1024       +1     
Flag Coverage Δ
unittests 82.35% <94.11%> (+0.02%) :arrow_up:

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

Files Changed Coverage Δ
echopype/convert/set_groups_base.py 86.70% <88.88%> (+0.13%) :arrow_up:
echopype/convert/set_groups_azfp.py 97.67% <100.00%> (+0.05%) :arrow_up:
echopype/convert/set_groups_ek60.py 100.00% <100.00%> (ø)
echopype/convert/set_groups_ek80.py 97.57% <100.00%> (+0.01%) :arrow_up:

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

leewujung commented 1 year ago

Ok, I ended up discovering more cases like this using the EK80 file. To summarize, these are:

Since they all can be handled in the same way, I added a new private method SetGroupsBase._nan_timestamp_handler for correcting a single nan value timestamp.

@emiliom: I changed your EK60 implementation to use the code I added for EK80, to account for cases where the channels may not ping simultaneously. I also changed the AZFP set_platform code to handle situations with missing timestamp via the _nan_timestamp_handler function.

leewujung commented 1 year ago

I'll self-merge this now to check everything is in good order for releasing v0.8.1.