search

OSOceanAcoustics / echopype

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

Re-order dimension of AZFP `echo_range` #752

Closed leewujung closed 1 year ago

leewujung commented 2 years ago

The order of dimension for echo_range under AZFP currently requires an ad-hoc change at the end of calibrate_azfp.py::_cal_power

out["echo_range"] = out["echo_range"].transpose("channel", "ping_time", "range_sample")

to make it consistent with the order for Sv, since this is not the case for other sonar models.

This seems better fixed in the AZFP part of compute_range so that we don't have to do this ad-hoc fix in this current place.

leewujung commented 2 years ago

@b-reyes has the fix here: https://github.com/OSOceanAcoustics/echopype/pull/736#discussion_r912285257

emiliom commented 1 year ago

(Commenting here rather than in #968, as it's a broader comment)

I don't think I follow why doing the dimension transpose in calibrate_azfp.py::_cal_power is more or less ad-hoc than doing it in range.py::compute_range_AZFP. The general need to ensure that the dimension order is always the same regardless of the sensor type is obvious. But where should that happen?

I see that a dimension transpose is done on EK data, too, in range.py::compute_range_EK: https://github.com/OSOceanAcoustics/echopype/blob/ea84663fb4e045a850a840aa0b012977ef65925c/echopype/calibrate/range.py#L173-L174 The comment above the code is helpful.

So, the reason for transposing dimensions for AZFP is not strictly to mirror EK, but for all sensors to adhere to a common convention that in turn is driven by the order of backscatter data.

I don't think I follow why the transpose step needs to happen in range.py rather than calibrate_*.py, for either sensors, but I definitely think the step should be applied in the same module category (range.py or calibrate_EK.py and calibrate_AZFP.py).

leewujung commented 1 year ago

I don't think I follow why the transpose step needs to happen in range.py rather than calibrate_*.py, for either sensors, but I definitely think the step should be applied in the same module category (range.py or calibrate_EK.py and calibrate_AZFP.py).

I think it is better to be in range.py because it is much easier to track bugs down or add new functionalities related to echo range in an independent script instead of going through the rabbit holes in calibrate_*.py. Note that in previous PRs I spent a lot of time to disentangle the range computation and related env param routines from calibrate_*.py.

emiliom commented 1 year ago

Ok, that's reasonable. Thanks.

leewujung commented 1 year ago

I see that a dimension transpose is done on EK data, too, in range.py::compute_range_EK

I forgot to respond to this part -- interesting. I hadn't realized that. I wonder why that was the case. Might be because the backscatter data is of that dimension. This may be something we revisit later to see if there's any difference in terms of efficiency. Maybe something along the line of pydantic would be useful here.

leewujung commented 1 year ago

I wonder why that was the case. Might be because the backscatter data is of that dimension.

This is exactly why because of the indexing needed to set echo_range entries that correspond to backscatter entries with NaN values to also be NaN.

leewujung commented 1 year ago

This issues is addressed in #968. I'll close this now.