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, mandatory Beam_groupX variables #1103

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

emiliom
Copy link
Collaborator

@emiliom emiliom commented Jul 31, 2023

Addresses #1097. Adds mandatory variables

@emiliom emiliom added the data format Anything about data format label Jul 31, 2023
@emiliom emiliom added this to the 0.8.0 milestone Jul 31, 2023
@emiliom emiliom requested a review from leewujung July 31, 2023 04:24
@emiliom emiliom marked this pull request as draft July 31, 2023 04:24
@emiliom emiliom self-assigned this Jul 31, 2023
@emiliom
Copy link
Collaborator Author

emiliom commented Jul 31, 2023

I've added sample_time_offset and removed range_sample_offset where it occurred (EK60 and EK80). range_sample_offset was recently added in PR #1099. For EK80, PR #1099 added a brand new variable. For EK60, it renamed the existing offset variable.

@emiliom emiliom marked this pull request as ready for review July 31, 2023 19:02
@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2023

Codecov Report

Merging #1103 (837dd2e) into dev (de4fb4d) will decrease coverage by 3.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #1103      +/-   ##
==========================================
- Coverage   78.14%   75.06%   -3.08%     
==========================================
  Files          65       23      -42     
  Lines        6232     3393    -2839     
==========================================
- Hits         4870     2547    -2323     
+ Misses       1362      846     -516     
Flag Coverage Δ
unittests 75.06% <ø> (-3.08%) ⬇️

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

Files Changed Coverage Δ
echopype/convert/set_groups_azfp.py 97.61% <ø> (ø)
echopype/convert/set_groups_ek60.py 92.74% <ø> (ø)
echopype/convert/set_groups_ek80.py 97.39% <ø> (ø)
echopype/utils/coding.py 29.88% <ø> (-59.78%) ⬇️

... and 48 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.

@emiliom : Thanks, looks good! I only have one question about the coding:

"beam_stabilisation": np.byte,
"non_quantitative_processing": np.int16,

Since they are both scalar and take 0 or 1 values, why is one np.byte and np.int16?

@emiliom
Copy link
Collaborator Author

emiliom commented Aug 2, 2023

Thanks!

beam_stabilisation": np.byte,
"non_quantitative_processing": np.int16,

Since they are both scalar and take 0 or 1 values, why is one np.byte and np.int16?

beam_stabilisation is defined to be only 0 or 1. But the definition for non_quantitative_processing is more open ended. I was being perhaps overly open to future possibilities in setting it to int16! But we could change it to byte, especially since in echopype we'll be assigning it a value of 0 in echopype in all cases. Update: actually, the convention does specify short for this variable, which is typically int16. That still seems like overkill, but since we're setting it to a scalar, its memory footprint is irrelevant.

@emiliom
Copy link
Collaborator Author

emiliom commented Aug 3, 2023

Thanks! Merging, based on our side conversation.

@emiliom emiliom merged commit e0e84f0 into OSOceanAcoustics:dev Aug 3, 2023
@emiliom emiliom deleted the beam-missing-mandatory-vars branch August 3, 2023 04:56
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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants