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 missing sampling_offset_time for EK60 & set EK60/80 beam_direction_x/y/z to nan as needed #1114

Merged
merged 2 commits into from
Aug 5, 2023

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented Aug 5, 2023

@emiliom emiliom added enhancement This makes echopype better data format Anything about data format labels Aug 5, 2023
@emiliom emiliom added this to the 0.8.0 milestone Aug 5, 2023
@emiliom emiliom requested a review from leewujung August 5, 2023 01:55
@emiliom emiliom self-assigned this Aug 5, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2023

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

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%) ⬆️
echopype/convert/set_groups_ek80.py 97.56% <100.00%> (+0.04%) ⬆️

... and 49 files with indirect coverage changes

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

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.

Thanks, these look good!

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. I wonder if this messiness is why convention ver.2 changed the definition to use angles in the spherical coordinate system.

Anyways, please feel free to merge!

@emiliom
Copy link
Collaborator Author

emiliom commented Aug 5, 2023

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.

@emiliom emiliom merged commit cd52645 into OSOceanAcoustics:dev Aug 5, 2023
5 checks passed
@emiliom emiliom deleted the beam_direction-nan branch August 5, 2023 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data format Anything about data format enhancement This makes echopype better
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants