Update compress_pulse to Parallelize Convolution #1208
Conversation
for more information, see https://pre-commit.ci
anantmittal
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
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! |
There was a problem hiding this comment.
@anantmittal: Thanks for the PR! The change here does make it possible to perform convolution on a dask array, which was previously not allowed, so that's great.
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. But this is probably not a fair comparison since the data is small. Were you able to try this on a much larger mock data (e.g. randomly generate backscatter_chan) to see what the difference may be?
Benchmarking testI did some simple benchmarking test with a test file in the
ResultsW/O Dask the run took 58.1 s Looking at the dask dashboard, the computation happens in parallel
For reproducibility, you can see my run in the following gist: https://gist.github.com/lsetiawan/20c09c3eaa76508a741e31ff28a83402 |
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 #1164. Further optimization can probably happen, but it's a first start 😄 |
|
@leewujung is this good to go? |
|
Yep, I think this is good now. Thanks for doing the benchmarking tests! |
* Bump actions/cache from 3.0.11 to 3.2.0 (#18) Bumps [actions/cache](https://github.com/actions/cache) from 3.0.11 to 3.2.0. - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](actions/cache@v3.0.11...v3.2.0) --- updated-dependencies: - dependency-name: actions/cache dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Update compress_pulse to Parallelize Convolution (#1208) Update compress_pulse to Parallelize Convolution. This addresses #1164. * Optimize `harmonize_env_param_time` (#1235) * Remove extra elses * Ensure harmonize_env_param_time works with dask arrays * Remove unused import * Optimize Frequency Differencing with Dask (#1198) * Update frequency_differencing method: add dask optimizations if Sv data is dask array * Update test_frequency_differencing function and incorporate test cases for dask * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix ci error * Update echopype/mask/api.py Co-authored-by: Don Setiawan <landungs@uw.edu> * Update echopype/mask/api.py: assign attrs out of if-else * Update echopype/mask/api.py: remove unused code * refactor: Update dask chunks iter to use xr.map_blocks * refactor: Cleanup code and make more explicit * Add better comments * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix pre-commit ci errors * refactor: Update algorithm to use map_blocks Update the looping algorithm to use dask array map_block instead. This would remove direct slicing ambiguity and prepare for future xarray map_block usage. * refactor: Modify to have no hardcoded channel slicing * refactor: Assign Sv data array --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Don Setiawan <landungs@uw.edu> * Optimize get vend cal params power (#1285) * Optimize get vend cal params power (#2) * Refactor function to avoid expand_dims and sortby if required * test: increase co-ordinates to 1000 for ping_time dimension * style: upper-casing variables * style: upper-casing variables * style: revert --------- Co-authored-by: Anant Mittal <anmittal@cs.washington.edu> * test: update test_get_interp_da() to test on 200 time_coordinates (#3) * test: refactor test data (#4) * test: refactor test data (#5) * test: refactor test data * test: refactor test data --------- Co-authored-by: Anant Mittal <anmittal@cs.washington.edu> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Anant Mittal <anmittal@cs.washington.edu> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Don Setiawan <landungs@uw.edu> Co-authored-by: anujsinha3 <jazzy.a30@gmail.com>
Explores #1164