[1488] Support frequency nominal dim in consolidate functions #1520

dbashford-NOAA · 2025-07-30T16:09:51Z

Resolves #1488

Use a generalized dim_0 so frequency nominal can be supported in place of channel
Added testing for the swapped dims
Update tests to use fixture for the test data folder

…ate tests

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

codecov-commenter · 2025-07-30T16:15:40Z

Codecov Report

❌ Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.95%. Comparing base (9f56124) to head (29e6153).
⚠️ Report is 230 commits behind head on main.

Files with missing lines	Patch %	Lines
echopype/consolidate/api.py	92.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1520      +/-   ##
==========================================
+ Coverage   83.52%   84.95%   +1.43%     
==========================================
  Files          64       79      +15     
  Lines        5686     7013    +1327     
==========================================
+ Hits         4749     5958    +1209     
- Misses        937     1055     +118

Flag	Coverage Δ
unittests	`84.95% <92.59%> (+1.43%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

leewujung · 2025-09-02T19:58:37Z

echopype/consolidate/api.py

    # and obtain the echodata group path corresponding to encode_mode
    ed_beam_group = retrieve_correct_beam_group(echodata, waveform_mode, encode_mode)

-    # check that source_Sv at least has a channel dimension
-    if "channel" not in source_Sv.variables:
-        raise ValueError("The input source_Sv Dataset must have a channel dimension!")
+    dim_0 = list(source_Sv.sizes.keys())[0]

-    # Select ds_beam channels from source_Sv
-    ds_beam = echodata[ed_beam_group].sel(channel=source_Sv["channel"].values)
+    if dim_0 in ["channel", "frequency_nominal"]:
+        ds_beam = echodata[ed_beam_group].sel({dim_0: source_Sv[dim_0].values})
+    else:
+        raise ValueError(
+            "The input source_Sv Dataset must have a channel or frequency_nominal dimension!"
+        )


In this implementation dim_0 can be anything that is in the first position (index 0) in the ds dataset keys. In theoryds can come from different sources, so there is no guarantee that the first key would be channel or frequency_nominal. Since you check if dim_0 in ["channel", "frequency_nominal"] downstream anyways, how about checking if the dimension contains one of these two options before proceeding?

I'm not sure I understand where the check should be. Instead of checking here are you saying it would be better earlier in the add_splitbeam_angle function?

i think it's more that dim_0 = list(source_Sv.sizes.keys())[0] may grab other dimensions if the order of dimension is different from having channel or frequency_nominal at the first position. It's better to search for those in the dimension list I think.

leewujung · 2025-09-02T19:59:58Z

echopype/consolidate/api.py

+            dim_0 = list(ds.sizes.keys())[0]
+            if echodata["Sonar/Beam_group1"][dim_0].equals(ds[dim_0]):
                beam_group_name = "Beam_group1"
            else:
                beam_group_name = "Beam_group2"


Also, the current implementation

if echodata["Sonar/Beam_group1"][dim_0].equals(ds[dim_0]): beam_group_name = "Beam_group1" else: beam_group_name = "Beam_group2"

is a weird setup in terms of Beam_groupX selection. This is a larger issue so no immediate actions required under this PR, but I will note this in a separate issue.

leewujung · 2025-09-02T20:12:32Z

echopype/tests/consolidate/test_consolidate_integration.py

+def test_add_splitbeam_angle_w_dim_swap(sonar_model, test_path_key, raw_file_name, test_path,
+                             paths_to_echoview_mat, waveform_mode, encode_mode,
+                             pulse_compression, to_disk):


This new test is largely identical with the existing test_add_splitbeam_angle, and the content of that test was to make sure the split beam angle computation is correct. I think it's a bit meddling the water to repeat the same thing here, since dimension swap is not part of the split beam calculate.

Could you slim this down to just testing the dimension swap?

There is already a test for the swap_dims_channel_frequency and I didn't make any changes to that function but do you think it needs more coverage? This test was meant to cover the changes I made to 'test_add_splitbeam_angle' and make sure the split beam angle computation was correct when the dims are swapped. Maybe it would be better to just add a dim swap test case to the existing test_add_splitbeam_angle test?

adding one test to go through the swap_dims_channel_frequency step and then add_splitbeam_angle would be great. I think making it a separate test from test_add_splitbeam_angle which tests the function outputs would be a good idea.

leewujung · 2025-09-02T20:14:13Z

echopype/tests/consolidate/test_add_depth.py

+def test_add_depth_EK_with_beam_angles_with_different_beam_groups_and_dim_swap(
+    file, sonar_model, compute_Sv_kwargs, expected_beam_group_name, ek80_path
+):


Similar to my comment on test_add_splitbeam_angle, I think it's better to not repeat the same test and create a new one focusing on the dimension swap.

leewujung

@dbashford-NOAA : Thanks for the PR. Sorry it took a while for me to get to this.

I made a few suggestions that I think will improve the robustness and the tests.

Could you also add a test for add_location to make sure things flow through even with dimension swap? Thanks.

Please request my review once you're done with the above.

leewujung · 2025-09-05T16:37:16Z

echopype/tests/consolidate/test_add_location.py

+    ds = ep.consolidate.swap_dims_channel_frequency(ds)
+    for group in ed["Sonar"]['beam_group'].values:
+        ed[f"Sonar/{group}"] = ep.consolidate.swap_dims_channel_frequency(ed[f"Sonar/{group}"])
+
+    ds_all = ep.consolidate.add_location(ds=ds, echodata=ed)


@dbashford-NOAA : these made me realize that, even though channel and frequency_nominal are not explicitly used in add_location, the interpolation would actually require the dimensions to be aligned for the function to work. The intention for the issue is to make all functions under ep.consolidate compatible with ds_Sv that already has the dimension swapped, so I thinks we should make the add_location able to handle this disjoint (between ed dimension channel and ds_Sv dimension frequency_nominal)? Then the function should run through with ds_all = ep.consolidate.add_location(ds=ds, echodata=ed) without having to swap the ed dimension.

Let me know if I misunderstood anything here.

To make sure I understand, you're saying that add_location should work with the ds dims swapped but without calling swap_dims_channel_frequency on each ed beam group? If so, should all ep.consolidate functions work that way?

(Somehow I missed your response...)

Yes I think all ep.consolidate functions should work that way. Basically the functions should work with either channel or frequency_nominal as a coordinate.

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

dbashford-NOAA and others added 2 commits July 30, 2025 09:44

[1488] Support frequency nominal dim in consolidate functions and upd…

7c0653d

…ate tests

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

2e1c814

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

dbashford-NOAA requested a review from leewujung July 30, 2025 16:41

dbashford-NOAA mentioned this pull request Jul 31, 2025

enh: add dimension alignment support for swap_dims_channel_frequency #1500

Closed

leewujung reviewed Sep 2, 2025

View reviewed changes

leewujung requested changes Sep 2, 2025

View reviewed changes

[1488] Create consolidate tests that include dim swapping

ed6c267

dbashford-NOAA requested a review from leewujung September 4, 2025 20:27

leewujung reviewed Sep 5, 2025

View reviewed changes

dbashford-NOAA and others added 5 commits September 26, 2025 13:06

[1488] Swap beam group dims in consolidate functions if needed

ff2638e

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

cb78b56

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

[1488] Fix f-string placeholder error

0a6842f

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

df6c82d

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

[1488] Re add ed ds compare for add_depth

29e6153

dbashford-NOAA requested a review from leewujung October 2, 2025 16:36

[1488] Support frequency nominal dim in consolidate functions #1520

Are you sure you want to change the base?

[1488] Support frequency nominal dim in consolidate functions #1520

Uh oh!

Conversation

dbashford-NOAA commented Jul 30, 2025

Uh oh!

codecov-commenter commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

leewujung Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leewujung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jul 30, 2025 •

edited

Loading

leewujung Sep 2, 2025 •

edited

Loading