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

Add missing sampling_offset_time for EK60 & set EK60/80 beam_direction_x/y/z to nan as needed #1114

Closed emiliom closed 1 year ago

emiliom commented 1 year ago
codecov-commenter commented 1 year ago

Codecov Report

Merging #1114 (fae819e) into dev (6671fec) will decrease coverage by 0.42%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1114      +/-   ##
==========================================
- Coverage   78.24%   77.83%   -0.42%     
==========================================
  Files          65       18      -47     
  Lines        6255     2950    -3305     
==========================================
- Hits         4894     2296    -2598     
+ Misses       1361      654     -707     
Flag Coverage Δ
unittests 77.83% <100.00%> (-0.42%) :arrow_down:

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

Files Changed Coverage Δ
echopype/convert/set_groups_ek60.py 93.02% <100.00%> (+0.28%) :arrow_up:
echopype/convert/set_groups_ek80.py 97.56% <100.00%> (+0.04%) :arrow_up:

... and 49 files with indirect coverage changes

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

emiliom commented 1 year ago

Thanks!

Thinking about it again, the (0, 0, 0) combination is invalid, but all combinations that do not give a length=1 vector would also be invalid, but I could see how people might put a non-normalized vector in the (x,y,z), so the check has to handle something else or determine whether to normalize the length by default, etc, etc.

Agreed. More generally, echopype doesn't do any conversion or checking (other than what I just added) of the ek60 and ek80 datagram values that are fed into beam_direction_x/y/z. That may be true for other parameters, too.