search

OSOceanAcoustics / echopype

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

Optimize Frequency Differencing with Dask #1198

Closed anantmittal closed 6 months ago

anantmittal commented 10 months ago

~Fixes #1165~

@leewujung update: This PR does not actually addresses #1165 (see https://github.com/OSOceanAcoustics/echopype/issues/1165#issuecomment-1776360813), but it optimizes the frequency-differencing operations by map_blocks, which is great.

codecov-commenter commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0bc534e) 77.04% compared to head (4215dd7) 93.00%. Report is 27 commits behind head on dev.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #1198 +/- ## =========================================== + Coverage 77.04% 93.00% +15.95% =========================================== Files 67 3 -64 Lines 5908 200 -5708 =========================================== - Hits 4552 186 -4366 + Misses 1356 14 -1342 ``` | [Flag](https://app.codecov.io/gh/OSOceanAcoustics/echopype/pull/1198/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OSOceanAcoustics) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/OSOceanAcoustics/echopype/pull/1198/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OSOceanAcoustics) | `93.00% <100.00%> (+15.95%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OSOceanAcoustics#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lsetiawan commented 10 months ago

@anantmittal Could you please fix the pre-commit failure. Thanks!

lsetiawan commented 9 months ago

@leewujung I know this is quite different from what you were looking for in https://github.com/OSOceanAcoustics/echopype/issues/1165#issuecomment-1776360813. I've created a separate issue for that in https://github.com/OSOceanAcoustics/echopype/issues/1200. Would be able to review this and see for this specific case that it's all good? Thanks!

lsetiawan commented 8 months ago

rather than assuming the 3rd dimension is range_sample, if the intention is to grab range_sample dimension, what about actually finding its index and use it to be error-proof? in terms of traversing the blocks, for larger combined datasets, usually the ping_time dimension will be the largest. I wonder if it is worthwhile to do this blocking along the ping_time dimension, especially if we're looking to change to use map_block in the future?

@leewujung I think I've addressed your concerns now, please let me know what you think. Thanks! 😄

lsetiawan commented 7 months ago

@leewujung is this good to go?

leewujung commented 6 months ago

@lsetiawan : Looks good. Thanks for the changes!