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

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

Merged
merged 5 commits into from
Sep 2, 2023

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented Sep 1, 2023

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.

…o the first ping_time value. Addresses xarray warnings.
@emiliom emiliom added this to the 0.8.1 milestone Sep 1, 2023
@emiliom emiliom self-assigned this Sep 1, 2023
@leewujung
Copy link
Member

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.

@leewujung leewujung changed the title Set EK60 Platform.time1 to first ping_time value when it's a single-valued nan [all tests ci] Set EK60/80 Platform.time1 to first ping_time value when it's a single-valued nan [all tests ci] Sep 2, 2023
leewujung added a commit to leewujung/echopype that referenced this pull request Sep 2, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2023

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%) ⬆️

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%) ⬆️
echopype/convert/set_groups_azfp.py 97.67% <100.00%> (+0.05%) ⬆️
echopype/convert/set_groups_ek60.py 100.00% <100.00%> (ø)
echopype/convert/set_groups_ek80.py 97.57% <100.00%> (+0.01%) ⬆️

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

@leewujung leewujung changed the title Set EK60/80 Platform.time1 to first ping_time value when it's a single-valued nan [all tests ci] Set EK60/80 Platform and NMEA nan timestamp to first ping_time value [all tests ci] Sep 2, 2023
@leewujung
Copy link
Member

leewujung commented Sep 2, 2023

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

  • EK60: Platform.time1
  • EK80: Platform.time1, Platform.time2
  • Platform/NMEA.time (this one did not show up for the OOI files because they happen to have a single, non-GPS NMEA message)

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 added a commit to leewujung/echopype that referenced this pull request Sep 2, 2023
@leewujung leewujung self-assigned this Sep 2, 2023
@leewujung
Copy link
Member

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

@leewujung leewujung merged commit 5133fc6 into OSOceanAcoustics:dev Sep 2, 2023
5 checks passed
@emiliom emiliom deleted the 0.8-warnings branch September 2, 2023 18:02
leewujung added a commit that referenced this pull request Sep 2, 2023
* add PRs

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* add #1154

* change #1154 PR title

* change date; more info re 3.11 test failures

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* small wording tweak

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants