Bring more consistency in the Platform group across sensors on conversion #1058

emiliom · 2023-06-07T23:44:29Z

I went through the behavior of each sensor type (EK60, EK80, AZFP) during conversion (open_raw) for the Platform group. There were a number of inconsistencies, some of which have been mentioned before in #740 and #1003. This PR also broadly addresses #1051, or creates a more consistent starting point for update_platform.

I've addressed most of the inconsistencies:

AZFP now produces largely the same set of variables as the other two sensors, though most of them are empty (nan). The exceptions are sentence_type, a non-convention variable found in EK60 and EK80; and tilt_x and tilt_y, non-convention variables only used with AZFP.
Modified the dimensionality of several variables in AZFP to match that in EK80.
Fixed some incorrect or missing long_name attributes, either through edits to convention/1.0.yml or via code changes.
Fixed an issue with EK60 where the global platform_* attributes were being dropped after initial inclusion.

I also made some cosmetic edits to all three sensor types to use similar code patterns and order of variables and coordinates, to improve clarity and consistency across sensors.

I ran manual tests but didn't run the test suite locally. If the CI exposes some new errors resulting from these changes, I'll update the PR.

Some open areas for improvement (but possibly not for 0.7.2):

In EK80, several variables are being populated with 0.0 where I suspect they should be Nan. I think this is happening in the parser code, but I only focused on the set_group_* modules.
EK60 adds the channel dimension to a number of variables for which EK80 doesn't, and EK80 adheres more closely to the convention with respect to variable dimensionality. I think the presence of the extra channel dimension is a mistake, but wanted to first double check with @leewujung before removing that dimension.
The convention specifies that water_level is a scalar and does not have a time dimension; time variability is found in vertical_offset instead, which in a ship corresponds to heave. But we've been assigning a time3 dimension to water_level.
More broadly, it looks like the convention doesn't require the creation of empty (all Nan) variables when their "obligation" flag is not "Mandatory" (M). See this protracted discussion and my own recent question at the end. It'll be up to us to decide whether to adopt a general practice for these cases. For reference, none of the variables in the Platform group are "Mandatory"!

…atform sentence_type

…fixed or added missing long_name for two variables

…ted; added long_name to sentence_type

…riables, filled with nan, following dimensionality as found in EK80; cosmetic changes for clarity and consistency

… scalar

codecov-commenter · 2023-06-08T00:35:50Z

Codecov Report

Merging #1058 (304e83f) into dev (7a49cc7) will decrease coverage by 0.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1058      +/-   ##
==========================================
- Coverage   78.19%   77.93%   -0.27%     
==========================================
  Files          67       18      -49     
  Lines        6192     2918    -3274     
==========================================
- Hits         4842     2274    -2568     
+ Misses       1350      644     -706

Flag	Coverage Δ
unittests	`77.93% <100.00%> (-0.27%)`	⬇️

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

Impacted Files	Coverage Δ
echopype/convert/set_groups_azfp.py	`97.61% <100.00%> (+0.15%)`	⬆️
echopype/convert/set_groups_ek60.py	`92.74% <100.00%> (+0.17%)`	⬆️
echopype/convert/set_groups_ek80.py	`97.04% <100.00%> (+0.02%)`	⬆️

... and 51 files with indirect coverage changes

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

emiliom · 2023-06-08T01:12:20Z

Alright, the tests are passing now! The only failure was with AZFP. check_platform_required_vars was actually checking for a set of variables that were all scalar; I removed the variables that are no longer scalar.

echopype/convert/set_groups_azfp.py

echopype/convert/set_groups_ek60.py

Co-authored-by: Wu-Jung Lee <[email protected]>

emiliom · 2023-06-08T19:08:07Z

Additional changes I'll add to this PR:

Remove time3 dimension from water_level
Remove inappropriate / incorrect channel dimension from several variables in EK60

…ariables; set param_* global attributes to empty string (removed use of ui_param); and cosmetic tweaks

…t, driven by water_level (to adhere to convention where it's a scalar); set Platform platform_* global attrs to empty string

… as a variable at the start for clarity

…to common-platform-vars

emiliom · 2023-06-09T02:24:39Z

Remove time3 dimension from water_level

Done. Removed time3 completely from the Platform group, for all 3 sensor types. In addition to water_level, 3 additional, non-convention EK80 variables became scalar.

Remove inappropriate / incorrect channel dimension from several variables in EK60

Done.

emiliom · 2023-06-09T02:26:27Z

I suspect some tests will now fail, but we'll see. I again ran into errors building the local test environment, so I couldn't run the test suite locally.

This PR will be ready for a final review once I see what CI tests fail, and fix the tests.

…lso that drop_keel_offset valid values are now handled correctly after replacing hasattr with dict .get in ek80 Platform set_group

emiliom · 2023-06-09T06:22:16Z

Only ek80 convert tests were failing. I've fixed them by addressing two changes:

Removal of time3 dimension from Platform group
A change I made in set_group_ek80.py originally for code readability actually fixed an unrecognized bug. For drop_keel_offset, a scheme involving hasattr was incorrectly being used on a dictionary, resulting in np.nan always being assigned even when there was valid data present. This was fixed by using the dictionary get method, and the fix required a change to a test.

emiliom · 2023-06-09T06:28:57Z

Woo-hoo, all tests are passing! This PR is ready for a final review, and hopefully merging soon after.

echopype/tests/convert/test_convert_ek80.py

leewujung

@emiliom : thanks for the changes! Everything looks fine. I just have a question about the test in test_convert_ek80, since the dimension do not match what we intended for the data variables, and the assert does not check the actual values. I know that this is an inherited test, but to avoid confusion later, I think we should fix it. (Or clarify what's going on if I am reading the code wrong -- I have been really slow this morning.)

echopype/tests/convert/test_convert_ek80.py

emiliom · 2023-06-10T17:57:23Z

NOTE: The build CI failed. Looking into it, I realized that I forgot that the PR CI by default runs in the selective mode so that only "convert" tests were run; plus b/c I was having problems with running tests locally, I never ran the full suite.

So, there are apparently more fixes to make resulting from the changes in this PR. From https://github.com/OSOceanAcoustics/echopype/actions/runs/5226125815/jobs/9436435332, the failures are in:

tests/calibrate/test_calibrate.py::test_env_params
tests/utils/test_processinglevels_integration.py::test_raw_to_mvbs[AZFP-AZFP-raw_and_xml_paths1-extras1]
tests/visualize/test_plot.py::test_water_level_echodata[True-False]

leewujung · 2023-06-10T18:43:18Z

Ah that's too bad! Hopefully these are easy to fix.

emiliom · 2023-06-10T18:49:14Z

After a quick look, two out of three seem easy. The 3rd one, I'm not sure yet.

… [all tests ci] (#1061) * Fix processing level L1A test to account for AZFP open_raw now creating empty lat & lon variables * Remove obsolete environmental parameters tests * Fix test_plot tests and visualize api involving water_level > vertical_offset and the removal of channel dim from ek60 platform vars * Cosmetic change simply to trigger all ci tests * Remove remnant test case for ek60 vertical_offset channel dim; correct data-type typing hint

…sion (OSOceanAcoustics#1058) * Update convention yaml with corrected long_name entries and adding platform sentence_type * EK80 Platform updates: cosmetic changes for clarity and consistency; fixed or added missing long_name for two variables * EK60 Platform updates: ensured platform_* global attributes are inserted; added long_name to sentence_type * AZFP Platform updates: added three coordinates; added many missing variables, filled with nan, following dimensionality as found in EK80; cosmetic changes for clarity and consistency * Remove from AZFP Platform test check the variables that are no longer scalar * When azfp tilt_x and tilt_y are all nan, create single-valued arrays * Update echopype/convert/set_groups_azfp.py Co-authored-by: Wu-Jung Lee <[email protected]> * For EK60 Platform, removed incorrect channel dimension from several variables; set param_* global attributes to empty string (removed use of ui_param); and cosmetic tweaks * Removed time3 from Platform and removed it from all variables using it, driven by water_level (to adhere to convention where it's a scalar); set Platform platform_* global attrs to empty string * In AZFP Platform group, change tilt units to arc_degree and set time1 as a variable at the start for clarity * Update ek80 convert tests to reflect removal of Platform time3; and also that drop_keel_offset valid values are now handled correctly after replacing hasattr with dict .get in ek80 Platform set_group * Update echopype/tests/convert/test_convert_ek80.py --------- Co-authored-by: Wu-Jung Lee <[email protected]>

…ceanAcoustics#1058 [all tests ci] (OSOceanAcoustics#1061) * Fix processing level L1A test to account for AZFP open_raw now creating empty lat & lon variables * Remove obsolete environmental parameters tests * Fix test_plot tests and visualize api involving water_level > vertical_offset and the removal of channel dim from ek60 platform vars * Cosmetic change simply to trigger all ci tests * Remove remnant test case for ek60 vertical_offset channel dim; correct data-type typing hint

emiliom added 4 commits June 7, 2023 15:52

Update convention yaml with corrected long_name entries and adding pl…

4a4ebd2

…atform sentence_type

EK80 Platform updates: cosmetic changes for clarity and consistency; …

0b0caa9

…fixed or added missing long_name for two variables

EK60 Platform updates: ensured platform_* global attributes are inser…

9269cbb

…ted; added long_name to sentence_type

AZFP Platform updates: added three coordinates; added many missing va…

b7e0a6f

…riables, filled with nan, following dimensionality as found in EK80; cosmetic changes for clarity and consistency

emiliom added the data format Anything about data format label Jun 7, 2023

emiliom added this to the 0.7.2 milestone Jun 7, 2023

emiliom requested a review from leewujung June 7, 2023 23:44

Remove from AZFP Platform test check the variables that are no longer…

40065b8

… scalar

When azfp tilt_x and tilt_y are all nan, create single-valued arrays

8aa4d9f

leewujung reviewed Jun 8, 2023

View reviewed changes

echopype/convert/set_groups_azfp.py Outdated Show resolved Hide resolved

leewujung reviewed Jun 8, 2023

View reviewed changes

echopype/convert/set_groups_azfp.py Show resolved Hide resolved

leewujung reviewed Jun 8, 2023

View reviewed changes

echopype/convert/set_groups_ek60.py Outdated Show resolved Hide resolved

Update echopype/convert/set_groups_azfp.py

6997f14

Co-authored-by: Wu-Jung Lee <[email protected]>

emiliom added 4 commits June 8, 2023 18:07

For EK60 Platform, removed incorrect channel dimension from several v…

064d0cd

…ariables; set param_* global attributes to empty string (removed use of ui_param); and cosmetic tweaks

Removed time3 from Platform and removed it from all variables using i…

ddf0a82

…t, driven by water_level (to adhere to convention where it's a scalar); set Platform platform_* global attrs to empty string

In AZFP Platform group, change tilt units to arc_degree and set time1…

be8c40d

… as a variable at the start for clarity

Merge branch 'common-platform-vars' of github.com:emiliom/echopype in…

439819b

…to common-platform-vars

emiliom mentioned this pull request Jun 9, 2023

Remove ui_param EchoData property #1059

Closed

Update ek80 convert tests to reflect removal of Platform time3; and a…

be26382

…lso that drop_keel_offset valid values are now handled correctly after replacing hasattr with dict .get in ek80 Platform set_group

leewujung reviewed Jun 9, 2023

View reviewed changes

echopype/tests/convert/test_convert_ek80.py Show resolved Hide resolved

leewujung reviewed Jun 9, 2023

View reviewed changes

emiliom commented Jun 9, 2023

View reviewed changes

echopype/tests/convert/test_convert_ek80.py Outdated Show resolved Hide resolved

Update echopype/tests/convert/test_convert_ek80.py

304e83f

emiliom merged commit 5d48a67 into OSOceanAcoustics:dev Jun 9, 2023

emiliom deleted the common-platform-vars branch June 9, 2023 21:15

This was referenced Jun 11, 2023

Additional test and related fixes stemming from Platform group PR #1058 [all tests ci] #1061

Merged

update_platform overhaul [all tests ci] #1060

Merged

leewujung modified the milestones: 0.7.2, 0.8.0 Jul 15, 2023

This was referenced Jul 31, 2023

AZFP: add missing mandatory Beam_group1 variables #1102

Merged

echodata.update_platform adds a water_level with incorrectly assigned time1 dimension #1003

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring more consistency in the Platform group across sensors on conversion #1058

Bring more consistency in the Platform group across sensors on conversion #1058

emiliom commented Jun 7, 2023 •

edited

Loading

codecov-commenter commented Jun 8, 2023 •

edited

Loading

emiliom commented Jun 8, 2023

emiliom commented Jun 8, 2023

emiliom commented Jun 9, 2023

emiliom commented Jun 9, 2023 •

edited

Loading

emiliom commented Jun 9, 2023

emiliom commented Jun 9, 2023

leewujung left a comment

emiliom commented Jun 10, 2023 •

edited

Loading

leewujung commented Jun 10, 2023

emiliom commented Jun 10, 2023

Bring more consistency in the Platform group across sensors on conversion #1058

Bring more consistency in the Platform group across sensors on conversion #1058

Conversation

emiliom commented Jun 7, 2023 • edited Loading

codecov-commenter commented Jun 8, 2023 • edited Loading

Codecov Report

emiliom commented Jun 8, 2023

emiliom commented Jun 8, 2023

emiliom commented Jun 9, 2023

emiliom commented Jun 9, 2023 • edited Loading

emiliom commented Jun 9, 2023

emiliom commented Jun 9, 2023

leewujung left a comment

Choose a reason for hiding this comment

emiliom commented Jun 10, 2023 • edited Loading

leewujung commented Jun 10, 2023

emiliom commented Jun 10, 2023

emiliom commented Jun 7, 2023 •

edited

Loading

codecov-commenter commented Jun 8, 2023 •

edited

Loading

emiliom commented Jun 9, 2023 •

edited

Loading

emiliom commented Jun 10, 2023 •

edited

Loading