OSOceanAcoustics / echopype

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

Update compress_pulse to Parallelize Convolution #1208

Closed anantmittal closed 4 months ago

anantmittal commented 8 months ago

Explores #1164

codecov-commenter commented 8 months ago

Codecov Report

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

Comparison is base (63547fc) 83.54% compared to head (ec44c38) 88.91%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #1208 +/- ## ========================================== + Coverage 83.54% 88.91% +5.36% ========================================== Files 64 11 -53 Lines 5675 974 -4701 ========================================== - Hits 4741 866 -3875 + Misses 934 108 -826 ``` | [Flag](https://app.codecov.io/gh/OSOceanAcoustics/echopype/pull/1208/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/1208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OSOceanAcoustics) | `88.91% <100.00%> (+5.36%)` | :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 8 months ago

This current PR seem to make a difference in terms of parallelization. @leewujung would you be able to go through the changes and see if this is what you're looking for for this case? Thank you!

lsetiawan commented 7 months ago

Benchmarking test

I did some simple benchmarking test with a test file in the ek80/D20170912-T234910.raw. The test does the following:

  1. I read in this file and then duplicate the data across additional ping times, which expands the data from 271.2 MB to 5.3 GB.
  2. From here, I chunk the data across ping time and save to a file for consistency
  3. I read in the data back and run this PR's compress_pulse function that have been modified and looks like when run in parallel with dask, we do see some speed up compared to not.

Results

W/O Dask the run took 58.1 s With Dask the run took 24.3 s

Looking at the dask dashboard, the computation happens in parallel

Screenshot 2023-12-15 at 3 32 30 PM

For reproducibility, you can see my run in the following gist: https://gist.github.com/lsetiawan/20c09c3eaa76508a741e31ff28a83402

lsetiawan commented 7 months ago

I tested this using a not very large dataset (test_data/ek80/D20170912-T234910.raw, ~95 MB), and found that by forcing the backscatter_r/i variables to be dask arrays, the parallelization didn't seem to consistently improve the speed compare to straight in-memory computation.

For a small dataset, due to the dask overhead for creating task graph, it's probably better to load things in memory first then do the computation. When I did try this with the original data, sure enough a dask array is slower than just in memory computation.

The approach suggested in this PR works for both in-memory and dask array, which is really powerful since when the data is big, we can parallelize the convolution! This I think satisfy the issue in https://github.com/OSOceanAcoustics/echopype/issues/1164. Further optimization can probably happen, but it's a first start 😄

lsetiawan commented 5 months ago

@leewujung is this good to go?

leewujung commented 4 months ago

Yep, I think this is good now. Thanks for doing the benchmarking tests!