search

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

Review/change handling of negative `echo_range` in calibration outputs #737

Closed leewujung closed 1 year ago

leewujung commented 2 years ago

Right now we set all negative echo_range to 0 at the end of compute_range, which causes problems when trying to combine datasets (#578) https://github.com/OSOceanAcoustics/echopype/blob/12693a72badcf54a18408e694b72e7b0e6d48023/echopype/echodata/echodata.py#L473

This issue is a reminder to review this and change/fix related operations in calibration.

leewujung commented 1 year ago

See https://github.com/OSOceanAcoustics/echopype/pull/968#issuecomment-1464282197 for the divide by zero warnings on ek80_bb_complex and ek80_cw_power range tests.

emiliom commented 1 year ago

I'm seeing the divide-by-zero error on compute_Sv with all kinds of raw files, including ek60 from the hake survey. In https://github.com/OSOceanAcoustics/echopype/pull/968#issuecomment-1464282197 I noted it for only those two ek80 tests, but I can now see that when running the full test, pytest somehow only spits out those warnings after those tests. When running each test individually, all ek tests produce the warning; the azfp test doesn't.

BTW, this warning doesn't happen with 0.6.3.

emiliom commented 1 year ago

@leewujung I've isolated the source of these log10 warnings. They're all in calibrate_ek.py.

Strictly speaking, something like this for both tvg_mod_range and prx would eliminate the warnings:

x = x.where(x > 0, eps)

where eps is a very small positive value (eg, 0.00001). But, I don't know what impact that would have on the calculations. On the other hand, those invalid log10 applications must be leading to nan or -inf values right now ...

What do you suggest?

emiliom commented 1 year ago

Can we close this now? I'm not 100% sure that the exact scope of the issue was addressed by #986

leewujung commented 1 year ago

I think this is addressed -- reading the description again I am not sure why data combination has anything to do with the range dimension. But I am glad that the range computation was reviewed/revised when range computation was factored out, and the log10(0) or log10(neg values) warning issue was resolved.