OSOceanAcoustics / echopype

Enabling interoperability and scalability in ocean sonar data analysis
https://echopype.readthedocs.io/
Apache License 2.0
95 stars 73 forks source link

Squeeze out `beam` dimension in `compute_Sv/TS` outputs #644

Closed leewujung closed 2 years ago

leewujung commented 2 years ago

With the addition of the beam dimension to a large number of variables in the Beam_groupX groups for compliance with the convention, now the outputs of compute_Sv/TS would also have a beam dimension. This dimension is of length=1 for AZFP, EK60 and EK80 power/angle data, but of length=4 for EK80 complex data.

The EK80 length=4 output does not make sense, since these are from a single split-beam transducers. There is a conceptual incongruence and potential confusion that takes place here. This is related to how we use the beam dimension to store data from each element of a split-beam transducer. (See https://github.com/OSOceanAcoustics/echopype/pull/638#issuecomment-1105807694 and https://github.com/OSOceanAcoustics/echopype/pull/638#issuecomment-1106744630 for detail)

The consensus was to squeeze out this extra beam dimension at the outputs of computer_Sv/TS.

We need to decide on our approach:

Having done some of option 1 (those are "saved" in a local branch now), option 2 seems a cleaner approach. However it probably would have an impact computationally. 🤔

emiliom commented 2 years ago

Hmm. I don't think I can add anything w/o evaluating the code. It sounds like it's a decision where the down sides are code complexity (option 1) vs computational performance (option 2)? How about if you push your local branch to your fork, and point us to the code you've changed?

For reference, the test @b-reyes reported in https://github.com/OSOceanAcoustics/echopype/pull/638#issuecomment-1106744630 https://github.com/OSOceanAcoustics/echopype/pull/638#issuecomment-1106744630 is tests/calibrate/test_calibrate.py::test_compute_Sv_ek80_matlab

emiliom commented 2 years ago

What did we decide about this, for v0.6.0?

leewujung commented 2 years ago

Ugh, forgot to come back to this. I think we decided to go with option 2, but it looks like that I have an action item to push up some option 1 attempt. Let me do that shortly. But looking at this again, it seems that just squeezing out that dimension at the end is more convenient. Once we get through v0.6.0 we can then work on the computational efficiency part.

Update: It seems that my changes were force-pushed (and hence removed) since it was a direct push to the upstream branch in https://github.com/OSOceanAcoustics/echopype/pull/638. I'll just go with option 2 to see how things look, and if necessary, switch to option1.

emiliom commented 2 years ago

We can close this now.