Add missing, mandatory `Beam_groupX` variables

emiliom commented 1 year ago

Addresses #1097. Adds mandatory variables

[x] beam_stabilisation, as a scalar
[x] non_quantitative_processing, as a scalar
[x] sample_time_offset: Will replace range_sample_offset, at least for EK60 & EK80. See https://github.com/OSOceanAcoustics/echopype/issues/1097#issuecomment-1656418738, and note that this change revisits issue #951

emiliom commented 1 year ago

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.

codecov-commenter commented 1 year ago

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%)`	:arrow_down:

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%)`	:arrow_down:

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

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 commented 1 year ago

Thanks! Merging, based on our side conversation.

OSOceanAcoustics / echopype

Add missing, mandatory `Beam_groupX` variables #1103

Codecov Report