[1488] Support frequency nominal dim in consolidate functions

[dbashford-NOAA]

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

[codecov-commenter]

Codecov Report

❌ Patch coverage is 70.26379% with 124 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.96%. Comparing base (9f56124) to head (1b30b30).
⚠️ Report is 387 commits behind head on main.

Files with missing lines	Patch %	Lines
...hopype/mask/seafloor_detection/bottom_blackwell.py	14.89%	40 Missing ⚠️
...hopype/clean/transient_noise/transient_fielding.py	38.33%	37 Missing ⚠️
echopype/mask/seafloor_detection/utils.py	53.84%	18 Missing ⚠️
...chopype/clean/transient_noise/transient_matecho.py	78.57%	15 Missing ⚠️
echopype/mask/shoal_detection/shoal_echoview.py	90.16%	6 Missing ⚠️
echopype/mask/shoal_detection/shoal_weill.py	91.22%	5 Missing ⚠️
echopype/consolidate/api.py	90.00%	1 Missing ⚠️
echopype/echodata/echodata.py	80.00%	1 Missing ⚠️
echopype/mask/api.py	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1520      +/-   ##
==========================================
+ Coverage   83.52%   84.96%   +1.44%     
==========================================
  Files          64       79      +15     
  Lines        5686     7004    +1318     
==========================================
+ Hits         4749     5951    +1202     
- Misses        937     1053     +116     

Flag	Coverage Δ
unittests	84.96% <70.26%> (+1.44%)	⬆️

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]

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]

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?

[dbashford-NOAA]

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?

[leewujung]

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]

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]

The block would fail if the number of channels/transducers in the 2 beamgroups are the same.

I think a cleaner approach is to simply check if dim_0 is "channel" or frequency_nominal, since this is within echopype and the first dimension should be one of these 2, or the function should error out. Later in line 225-226 you explicitly do an if-else for these 2 allowed dimension names anyway, so no need to broadening this up to other possible dimension names.

[leewujung]

adapt these to work with direct check of if dim_0 is "channel" or "frequency_nominal"

[leewujung]

We don't want to swap "frequency_nominal" with "channel" -- we want to preserve whatever the dimension the input ds has and just add the depth variable to the dataset.

[leewujung]

We don't want to swap "frequency_nominal" with "channel" -- we want to preserve whatever the dimension the input ds has and just add the depth variable to the dataset.

[leewujung]

make sure to change this too since we want to preserve whatever the input ds has

[leewujung]

make sure to change this too since we want to preserve whatever the input ds has

[leewujung]

@dbashford-NOAA : Sorry it again took a while for me to get to this!

I think there may have been some misunderstandings on what #1488 aims to achieve, but hopefully my comments below and inline would clear it up.

The goal of #1488 is to enable users to use all functions in ep.consolidate on Sv dataset regardless of if they have called swap_dims_channel_frequency or not.

With this in mind, I have a couple suggestions for this PR:

• for all ep.consolidate functions, check if either "channel" or "frequency_nominal" exists in the input ds, and error out if not.
  • I'd suggest making a small utility function for this and returns dim_0, so that the same routine can be used across all functions. This would help with future maintenance since everything is in one place.
• make sure all ep.consolidate functions have a test that checks if the function would run through on Sv dataset both before and after calling swap_dims_channel_frequency -- this embodies the goal of #1488

Also, please pull in all upstream changes to make sure your tests will run with the right assets. We've changed the CI setup pretty significantly in the past month or so. Those changes shouldn't impact the components you're working on, but just in case since I saw some directory changes in this PR.

[dbashford-NOAA]

@leewujung
Do you know if there is any test data I could use for add_depth and add_splitbeam_angle that has the frequency_nominal dim instead of channel?
I think part of the issue is that I was trying to adapt the existing test data to test with a frequency_nominal dim.

[leewujung]

@dbashford-NOAA : ah I see. Echopype generated echodata object and its derivative like the Sv dataset would always have channel as a dimension initially. It’s only when users use swap_dims_channel_frequency will they get that dimension changed to frequency_nominal. We made that swap function because most people refer to different channels by their central frequency - like “the 120 kHz channel.”

What we can’t control is when users would do the swap, and therefore we want to get all consolidate functions to be able to work with both the before and after swap data.

So, you can get that by using the swap function.

[dbashford-NOAA]

@leewujung
I think I've addressed everything you mentioned. I left the dim swap in the test code just as a way to adapt the data to test when dim_0 is frequency_nominal. Let me know if there's anything else that needs to change.