Move Beam group to Sonar/Beam_group1 and Beam_power to Sonar/Beam_power #574

emiliom · 2022-03-05T22:44:58Z

See #519. Also some relevant discussions in PR #545, which was closed w/o merging; this PR is based on that older PR.

This is only a first, incremental step to fully supporting a flexible Sonar/Beam_groupX multi-beam-group approach. Some pre-existing hard wiring of beam group names should ultimately be generalized.

For EK80 I only moved Beam_power to Sonar/Beam_power. I didn't want to take the time to change it to Sonar/Beam_group2, for now.

#567 is not addressed at all in this PR.

…from Google Drive

…/Beam_power

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

codecov-commenter · 2022-03-05T23:01:34Z

Codecov Report

Merging #574 (775c960) into dev (953a8d2) will decrease coverage by 7.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #574      +/-   ##
==========================================
- Coverage   78.59%   71.39%   -7.20%     
==========================================
  Files          40       22      -18     
  Lines        3499     2678     -821     
==========================================
- Hits         2750     1912     -838     
- Misses        749      766      +17

Flag	Coverage Δ
unittests	`71.39% <100.00%> (-7.20%)`	⬇️

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

Impacted Files	Coverage Δ
echopype/echodata/echodata.py	`65.38% <ø> (-25.65%)`	⬇️
echopype/convert/api.py	`87.60% <100.00%> (+0.10%)`	⬆️
echopype/convert/set_groups_azfp.py	`98.27% <100.00%> (+0.03%)`	⬆️
echopype/convert/set_groups_base.py	`85.52% <100.00%> (-1.98%)`	⬇️
echopype/convert/set_groups_ek60.py	`91.26% <100.00%> (+0.08%)`	⬆️
echopype/convert/set_groups_ek80.py	`96.10% <100.00%> (+0.07%)`	⬆️
echopype/echodata/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
echopype/echodata/convention/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
echopype/echodata/convention/conv.py	`11.11% <0.00%> (-88.89%)`	⬇️
echopype/echodata/api.py	`16.66% <0.00%> (-66.67%)`	⬇️
... and 23 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

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

emiliom · 2022-03-09T04:50:58Z

I've expanded the scope of this PR a bit by pushing a commit that adds the new variables beam_group_name and beam_group_descr to the Sonar group, which @leewujung and I discussed earlier. I've added them for EK60, EK80 and AZFP. It's an initial implementation, but it also includes the sensor-specific specification of group names and descriptions within the set_groups_<sensor>.py modules. It addresses some of my comments in #519 (comment)

@leewujung and @lsetiawan, it'd be great if you could review this PR 🥺

leewujung

Thanks @emiliom for this heavy lifting!! My comments are mostly about the subgroup name (/Sonar/Beam_group2 vs /Sonar/Beam_power) and the descriptions.

How about we remove echopype/convert/add_ancillary.py so that it does not cause confusion? It is not used by anything else.

From a quick skim through of the actual update_platform(), I don't immediately see change needed to make it work with the new under-the-hood change connection with the files.

leewujung · 2022-03-10T16:29:20Z

echopype/convert/api.py

@@ -171,7 +173,7 @@ def _save_groups_to_file(echodata, output_path, engine, compress=True):
            path=output_path,
            mode="a",
            engine=engine,
-            group="Beam_power",
+            group="Sonar/Beam_power",


What do you think about just making this "Sonar/Beam_group2" to be consistent with the convention?

Currently ed.beam_power only exists if there is also complex data in the .raw file (which are stored in ed.beam as the "default"). Otherwise there is only ed.beam.
So I am proposing is effectively making this mapping

(in EchoData object) ed.beam -- (in file) "/Sonar/Beam_group1"

(in EchoData object) ed.beam_power -- (in file) "/Sonar/Beam_group2"

oops sorry for womansplaining you -- I see from below that you are VERY aware of what data exists and when. :)

What do you think about just making this "Sonar/Beam_group2" to be consistent with the convention?

I can do that. I was just trying to include in this PR the core of my proposed changes. So, I had decided to not bother initially with the busy work of changing Beam_power to Beam_group2. I also wanted you and others to have a chance to think about whether having the mapping "(in EchoData object) ed.beam_power -- (in file) '/Sonar/Beam_group2' could be confusing.

leewujung · 2022-03-10T16:32:28Z

echopype/convert/add_ancillary.py

+def update_platform(
+        self,
+        files=None,
+        beam_group_name="Beam_group1",
+        extra_platform_data=None):


I think this is a deprecated function and the current update_platform is part of the EchoData class in

echopype/echopype/echodata/echodata.py

Line 387 in 5cf1153

def update_platform(

We should clean all of these up in v0.6.0! I was confused myself for a min...

leewujung · 2022-03-10T16:37:31Z

echopype/convert/set_groups_azfp.py

+        self._beamgroups = [
+            {
+                "name": "Beam_group1",
+                "descr": "contains complex backscatter data and other beam or channel-specific data.",
+            }
+        ]
+


AZFP only store power data and the numbers need to be converted before it is "power" with physical meaning, so maybe the description could be changed to something like
"descr": "contains backscatter power (uncalibrated) and other beam or channel-specific data." ?

Thanks. The core point of this PR is the implementation of the general mechanism to specify these descriptions as instrument-specific strings. The beauty is that once this is in place, we can tweak the "descr" easily, any time! But yeah, I'm happy to modify the PR later to replace the descr string with your suggestion.

leewujung · 2022-03-10T16:39:24Z

echopype/convert/set_groups_base.py

+            ),
+        }
+
+        return beam_groups_vars


I like this! :D

leewujung · 2022-03-10T16:42:46Z

echopype/convert/set_groups_ek60.py

+                "descr": "contains complex backscatter data and other beam or channel-specific data.",
+            }
+        ]
+


Similar to AZFP, EK60 does not record complex data. Depending on the transducer, it could be power data only (single beam) or power-angle data (split beam).
How about something like the following for the description?

"descr": ("contains backscatter power (uncalibrated) and other beam or channel-specific data, including split-beam angle data when they exist.")

(is data singular or plural in a convention??)

See my comment above about AZFP. And heh, that string was copied directly from 1.0.yml and it's what we've been using all along in the EchoData html repr 😆

(is data singular or plural in a convention??)

Heh. I like what this article says: "Data typically takes singular verbs and pronouns when writing for general audiences and in data journalism contexts: The data is sound. In scientific and academic writing, plural verbs and pronouns are preferred.". I have no idea if SONAR-netCDF4 is consistent about singular vs plural. I know I am not consistent on any given day, in general!

in one of the future docathon... search for "data" and make all self-consistent 😆

leewujung · 2022-03-10T16:44:31Z

echopype/convert/set_groups_ek80.py

+                "name": "Beam_power",
+                "descr": "contains backscatter data (power-only) and other beam or channel-specific data.",


Similar to EK60:
"descr": ("contains backscatter power (uncalibrated) and other beam or channel-specific data,
including split-beam angle data when they exist.")

leewujung · 2022-03-10T16:56:11Z

echopype/convert/set_groups_ek80.py

-                ds_beam_power = merge_save(ds_power, "power", group_name="Beam_power")
+                ds_beam_power = merge_save(
+                    ds_power, "power", group_name="/Sonar/Beam_power"
+                )


Similar to the above, making this "/Sonar/Beam_group2" ?

leewujung · 2022-03-10T16:57:14Z

echopype/echodata/convention/1.0.yml

+    name: Sonar/Beam_group1
    description: contains backscatter data and other beam or channel-specific data.


Suggested edits (feel free to modify!)

"description": ("contains backscatter data (either complex samples or uncalibrated power samples) and other beam or channel-specific data, including split-beam angle data when they exist.")

leewujung · 2022-03-10T17:01:16Z

echopype/echodata/convention/1.0.yml

+    name: Sonar/Beam Power
    description: contains backscatter data (power-only) and other beam or channel-specific data.


Something like below?

name: Sonar/Beam_group2 "description": ("contains backscatter power (uncalibrated) and other beam or channel-specific data, including split-beam angle data when they exist. Only exist if complex backscatter data is already in Sonar/Beam_group1")

for multiline yaml you could do:

description: | contains backscatter power (uncalibrated) and other beam or channel-specific data, including split-beam angle data when they exist. Only exist if complex backscatter data is already in Sonar/Beam_group1

leewujung · 2022-03-10T17:01:57Z

echopype/tests/echodata/test_echodata.py

    )


 @pytest.fixture(
    params=[
        single_ek60_zarr,
        (str, "ncei-wcsd", "Summer2017-D20170615-T190214.zarr"),
-        (None, "ncei-wcsd", "Summer2017-D20170615-T190214.nc"),
+        (None, "ncei-wcsd", "Summer2017-D20170615-T190214__NEW.nc"),


What is in this __NEW file?

Summer2017-D20170615-T190214.raw converted using the code in this PR, and therefore with the beam group in Sonar/Beam_group1 rather than Beam. See a Slack thread with @lsetiawan from 3/4 on the echopype channel.

emiliom · 2022-03-10T17:26:54Z

How about we remove echopype/convert/add_ancillary.py so that it does not cause confusion? It is not used by anything else.

Ah! I didn't try to see whether that module was still used. I was just replacing the occurrences of Beam references with Sonar/Beam_group1 or something like that. I love removing unused, deprecated stuff, so I'm happy to delete it in a commit to be added to this PR 😍

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

emiliom · 2022-03-12T04:53:52Z

I've implemented and pushed all the changes @leewujung suggested. I'll merge the PR once the tests pass successfully.

leewujung · 2022-03-12T05:28:08Z

This is exciting! 🎊

emiliom added 10 commits January 28, 2022 20:53

Move Beam group to Sonar/Beam

429b251

Move Beam group to Sonar/Beam

0cf9509

echodata tests now work with new nc test file

894b588

echodata tests now work with new nc test file

82cdefe

Merge branch 'beam_group' of github.com:emiliom/echopype into beam_group

3693fe0

Fix harmless mistake in a comment, in test

9c3a39a

Run TestEchodata on new nc file within the standard folder structure …

1ad41dd

…from Google Drive

Merge dev into beam_group

79df08d

Update test_echodata with sonar/beam nc and zarr datasets

810b673

Rename groups Sonar/Beam to Sonar/Beam_group1 and Beam_power to Sonar…

8b13e16

…/Beam_power

emiliom added the data format Anything about data format label Mar 5, 2022

emiliom added this to the 0.6.0 milestone Mar 5, 2022

emiliom requested a review from lsetiawan March 5, 2022 22:44

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

473758b

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

emiliom requested a review from leewujung March 5, 2022 22:45

emiliom mentioned this pull request Mar 8, 2022

Move Beam group to be a Sonar subgroup #519

Closed

emiliom and others added 2 commits March 8, 2022 20:42

Add new beam group variables to Sonar group

ceebca2

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

f2f4ebf

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

leewujung reviewed Mar 10, 2022

View reviewed changes

emiliom and others added 2 commits March 11, 2022 20:52

Changed Beam_power to Beam_group2 and updated Beam group descriptions

37c4501

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

775c960

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

emiliom merged commit aaf41ef into OSOceanAcoustics:dev Mar 12, 2022

emiliom mentioned this pull request Mar 13, 2022

Change water_level name to range_offset and include it in the returned Dataset from compute_SV #579

Closed

leewujung mentioned this pull request Mar 16, 2022

Fix flake8 linting issue that causes all the PR's to fail #587

Closed

emiliom mentioned this pull request Mar 29, 2022

Add coordinate values to SONAR-netCDF4 convention file #598

Closed

emiliom mentioned this pull request Mar 29, 2022

Modify SONAR-netCDF4 convention file #596

Closed

emiliom mentioned this pull request May 1, 2022

Overhaul and fix Sonar beam_group variables #658

Merged

emiliom deleted the beam_group branch December 24, 2022 03:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Beam group to Sonar/Beam_group1 and Beam_power to Sonar/Beam_power #574

Move Beam group to Sonar/Beam_group1 and Beam_power to Sonar/Beam_power #574

emiliom commented Mar 5, 2022 •

edited

Loading

codecov-commenter commented Mar 5, 2022 •

edited

Loading

emiliom commented Mar 9, 2022

leewujung left a comment •

edited

Loading

leewujung Mar 10, 2022

leewujung Mar 10, 2022

emiliom Mar 10, 2022

leewujung Mar 10, 2022

leewujung Mar 10, 2022

emiliom Mar 10, 2022

leewujung Mar 10, 2022

leewujung Mar 10, 2022

emiliom Mar 10, 2022 •

edited

Loading

leewujung Mar 10, 2022

emiliom Mar 10, 2022

leewujung Mar 10, 2022

leewujung Mar 10, 2022

leewujung Mar 10, 2022

leewujung Mar 10, 2022

lsetiawan Mar 10, 2022

leewujung Mar 10, 2022

emiliom Mar 10, 2022

emiliom commented Mar 10, 2022

emiliom commented Mar 12, 2022

leewujung commented Mar 12, 2022

		"name": "Beam_power",
		"descr": "contains backscatter data (power-only) and other beam or channel-specific data.",

		name: Sonar/Beam_group1
		description: contains backscatter data and other beam or channel-specific data.

		name: Sonar/Beam Power
		description: contains backscatter data (power-only) and other beam or channel-specific data.

Move Beam group to Sonar/Beam_group1 and Beam_power to Sonar/Beam_power #574

Move Beam group to Sonar/Beam_group1 and Beam_power to Sonar/Beam_power #574

Conversation

emiliom commented Mar 5, 2022 • edited Loading

codecov-commenter commented Mar 5, 2022 • edited Loading

Codecov Report

emiliom commented Mar 9, 2022

leewujung left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emiliom Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emiliom commented Mar 10, 2022

emiliom commented Mar 12, 2022

leewujung commented Mar 12, 2022

emiliom commented Mar 5, 2022 •

edited

Loading

codecov-commenter commented Mar 5, 2022 •

edited

Loading

leewujung left a comment •

edited

Loading

emiliom Mar 10, 2022 •

edited

Loading