Drop beam dimension #1056

emiliom · 2023-06-05T22:14:12Z

I'll leave this PR as a draft. For now, it's meant only for discussion, especially for @leewujung to see the minimal changes I made that fully remove the "unnecessary" use of the beam dimension for EK60 and AZFP data. ~~only, for now; I'll add AZFP later, where we're also dropping the beam dimension completely.~~ For EK80, the removal will not be complete, as the beam dimension will remain for a few variables.

Addresses #978

As this will be a breaking change, our intent is to release it with version 0.8.0.

echopype/convert/set_groups_ek60.py

leewujung

@emiliom : I checked the changes and they look fine. I think it makes more sense to review this once all changes for different sonar_models are in, since there are some redundant code in set_groups_base.py that needs changing too (I just found out).

Also, a quick note that please make sure for EK80 backscatter data and some parameters the beam dimension is removed too. There are also other parameters Thanks!

leewujung · 2023-06-06T15:19:18Z

Related to this, I just did #1057 to capture the other half of what #978 exposed but not specifically called out. The artificial expansion (repetition of the same value) along the ping_time dimension is more costly than the length=1 beam dimension when data is loaded into memory, and I think we should consider removing it at v0.8.0.

emiliom · 2023-06-06T17:27:31Z

@emiliom : I checked the changes and they look fine. I think it makes more sense to review this once all changes for different sonar_models are in, since there are some redundant code in set_groups_base.py that needs changing too (I just found out).

Yeah, for now I just wanted to make these changes visible to you.

Also, a quick note that please make sure for EK80 backscatter data and some parameters the beam dimension is removed too. There are also other parameters Thanks!

Yup, absolutely. That was the original scope of #978. I'm realizing now that we haven't either changed that scope there or created a new issue to specify that we're removing the beam dimension more broadly, wherever it's not "needed". I'll follow up.

I'll edit the PR description above to mention EK80. Initially, I was focusing on the cases (EK60 and AZFP) where the beam dimension will be dropped completely.

…dim from EK60 and AZFP

emiliom · 2023-07-14T21:46:06Z

Removed the beam dimension from AZFP
Updated all EK60 and AZFP tests to account for the removal of the beam dimension

…st output in export_engine

codecov-commenter · 2023-07-15T17:40:35Z

Codecov Report

Merging #1056 (d85d8c3) into dev (60916e0) will decrease coverage by 17.65%.
The diff coverage is 41.17%.

@@             Coverage Diff             @@
##              dev    #1056       +/-   ##
===========================================
- Coverage   78.12%   60.47%   -17.65%     
===========================================
  Files          67       46       -21     
  Lines        6253     5204     -1049     
===========================================
- Hits         4885     3147     -1738     
- Misses       1368     2057      +689

Flag	Coverage Δ
unittests	`60.47% <41.17%> (-17.65%)`	⬇️

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

Impacted Files	Coverage Δ
echopype/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
echopype/calibrate/calibrate_azfp.py	`94.87% <ø> (ø)`
echopype/calibrate/range.py	`87.69% <ø> (+1.53%)`	⬆️
echopype/consolidate/split_beam_angle.py	`78.37% <ø> (-11.24%)`	⬇️
echopype/convert/set_groups_azfp.py	`77.38% <0.00%> (-20.24%)`	⬇️
echopype/convert/set_groups_ek60.py	`75.00% <0.00%> (-17.75%)`	⬇️
echopype/convert/set_groups_ek80.py	`82.52% <0.00%> (-14.50%)`	⬇️
echopype/calibrate/cal_params.py	`92.36% <100.00%> (+0.45%)`	⬆️
echopype/calibrate/calibrate_ek.py	`98.38% <100.00%> (+0.51%)`	⬆️

... and 48 files with indirect coverage changes

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

…d only with complex samples

…ot present

…ve elimination of beam dim from EK80

emiliom · 2023-07-18T17:19:06Z

Removed the addition of beam dimension from EK80 when it's not actually required
Updated all EK80 tests to account for this change

…st output in export_engine

emiliom · 2023-07-19T19:35:07Z

Note: It's quite possible that in calibration modules there are some checks for the presence of the beam dimension that are no longer necessary, and I didn't catch that. For example, this block:

echopype/echopype/calibrate/calibrate_ek.py

Lines 579 to 584 in c5afb65

    
           # TODO: out should not have beam dimension at this stage 
        
           # once that dimension is removed from equivalent_beam_angle 
        
           if "beam" in out.coords: 
        
               return out.isel(beam=0).drop("beam") 
        
           else: 
        
               return out

echopype/calibrate/cal_params.py

echopype/calibrate/calibrate_ek.py

echopype/convert/set_groups_azfp.py

echopype/tests/echodata/test_echodata.py

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

…icitly

leewujung · 2023-07-20T05:10:05Z

I've caught the cases in calibration code where the beam dim already does not exist and hence does not need to be dropped anymore. Also reverted the if-else handling related to equivalent_beam_angle, which should not have a beam dimension to begin with.

There are few more cases where the beam dimension handling needs to be removed:

cal_parasm.py::_get_interp_da
test_cal_params.py::beam_AZFP
test_cal_params.py::test_get_interp_da test function inputs

…tive case

leewujung · 2023-07-22T05:16:03Z

I've added the remaining changes. Will merge this now to move forward to #1057.

emiliom · 2023-07-22T15:46:50Z

Great, thanks!!

* Remove beam dimension previously added during EK60 conversion * Handle case in EK calibration where beam dimension is not present * Remove beam dimension previously added during AZFP conversion * Remove handling of beam dimension in AZFP calibration * Update convert and echodata tests to account for elimination of beam dim from EK60 and AZFP * Forgot to undo in previous commit the temporary removal of netcdf4 test output in export_engine * Remove forced addition of beam dim in EK80 when not required. Now used only with complex samples * Remove forced handling of beam dim in EK80 calibration when beam is not present * Update convert, echodata and calibration tests to account for selective elimination of beam dim from EK80 * Forgot to undo in previous commit the temporary removal of netcdf4 test output in export_engine * fix test_nan_range_entries * fix test mock ata * remove added if-else * remove beam dim handling in calibrate/cal_params.py * revise comment re one place where the beam dim should be dropped explicitly * remove outdated comments re which var has beam dim * remove redundant check for if beam dim exists * remove beam dim from test_cal_params.py::beam_AZFP * remove beam dim check cal_params.py::_get_interp_da under the alternative case --------- Co-authored-by: Wu-Jung Lee <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

emiliom added 3 commits May 25, 2023 18:17

Remove beam dimension previously added during EK60 conversion

c589c35

Handle case in EK calibration where beam dimension is not present

09f033e

Merge branch 'dev' into drop_beam_dim

2445370

emiliom marked this pull request as draft June 5, 2023 22:14

leewujung reviewed Jun 6, 2023

View reviewed changes

echopype/convert/set_groups_ek60.py Show resolved Hide resolved

leewujung reviewed Jun 6, 2023

View reviewed changes

leewujung mentioned this pull request Jun 6, 2023

Remove redundant ping_time dimension #1057

Closed

Merge branch 'dev' into drop_beam_dim

39521e6

emiliom added the data format Anything about data format label Jul 14, 2023

emiliom added this to the 0.8.0 milestone Jul 14, 2023

emiliom added 3 commits July 14, 2023 14:38

Remove beam dimension previously added during AZFP conversion

3a0d892

Remove handling of beam dimension in AZFP calibration

86a833a

Update convert and echodata tests to account for elimination of beam …

996d939

…dim from EK60 and AZFP

Forgot to undo in previous commit the temporary removal of netcdf4 te…

111a3e1

…st output in export_engine

Merge branch 'dev' into drop_beam_dim

7c6766f

leewujung mentioned this pull request Jul 18, 2023

Draft v0.8.0 release notes #1063

Merged

emiliom added 3 commits July 18, 2023 10:10

Remove forced addition of beam dim in EK80 when not required. Now use…

acca64b

…d only with complex samples

Remove forced handling of beam dim in EK80 calibration when beam is n…

90a7315

…ot present

Update convert, echodata and calibration tests to account for selecti…

acbc8dd

…ve elimination of beam dim from EK80

Forgot to undo in previous commit the temporary removal of netcdf4 te…

d7cc8e9

…st output in export_engine

emiliom marked this pull request as ready for review July 19, 2023 17:20

leewujung reviewed Jul 19, 2023

View reviewed changes

echopype/calibrate/cal_params.py Outdated Show resolved Hide resolved

leewujung reviewed Jul 19, 2023

View reviewed changes

echopype/calibrate/calibrate_ek.py Outdated Show resolved Hide resolved

leewujung reviewed Jul 19, 2023

View reviewed changes

echopype/convert/set_groups_azfp.py Show resolved Hide resolved

leewujung reviewed Jul 19, 2023

View reviewed changes

echopype/tests/echodata/test_echodata.py Outdated Show resolved Hide resolved

leewujung and others added 8 commits July 19, 2023 16:21

fix test_nan_range_entries

4e011ab

fix test mock ata

cbed2ba

remove added if-else

6094ec3

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

43a9f92

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

remove beam dim handling in calibrate/cal_params.py

2f694ca

revise comment re one place where the beam dim should be dropped expl…

c53946d

…icitly

remove outdated comments re which var has beam dim

9869507

remove redundant check for if beam dim exists

4cf2c83

leewujung added 3 commits July 21, 2023 21:08

remove beam dim from test_cal_params.py::beam_AZFP

ffb090c

remove beam dim check cal_params.py::_get_interp_da under the alterna…

537197b

…tive case

Merge branch 'dev' into drop_beam_dim

d85d8c3

leewujung merged commit 9fda953 into OSOceanAcoustics:dev Jul 22, 2023

emiliom deleted the drop_beam_dim branch July 22, 2023 15:46

leewujung mentioned this pull request Jul 24, 2023

Remove beam dimension in EK80 angle-related calibration parameters #978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop beam dimension #1056

Drop beam dimension #1056

emiliom commented Jun 5, 2023 •

edited

Loading

leewujung left a comment •

edited

Loading

leewujung commented Jun 6, 2023

emiliom commented Jun 6, 2023

emiliom commented Jul 14, 2023

codecov-commenter commented Jul 15, 2023 •

edited

Loading

emiliom commented Jul 18, 2023

emiliom commented Jul 19, 2023

leewujung commented Jul 20, 2023

leewujung commented Jul 22, 2023 •

edited

Loading

emiliom commented Jul 22, 2023

Drop beam dimension #1056

Drop beam dimension #1056

Conversation

emiliom commented Jun 5, 2023 • edited Loading

leewujung left a comment • edited Loading

Choose a reason for hiding this comment

leewujung commented Jun 6, 2023

emiliom commented Jun 6, 2023

emiliom commented Jul 14, 2023

codecov-commenter commented Jul 15, 2023 • edited Loading

Codecov Report

emiliom commented Jul 18, 2023

emiliom commented Jul 19, 2023

leewujung commented Jul 20, 2023

leewujung commented Jul 22, 2023 • edited Loading

emiliom commented Jul 22, 2023

emiliom commented Jun 5, 2023 •

edited

Loading

leewujung left a comment •

edited

Loading

codecov-commenter commented Jul 15, 2023 •

edited

Loading

leewujung commented Jul 22, 2023 •

edited

Loading