hydro: drop twi dask+cupy host round-trip, vectorize watershed init loops#3694
Merged
Conversation
brendancol
commented
Jul 23, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: hydro: drop twi dask+cupy host round-trip, vectorize watershed init loops
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
xrspatial/hydro/twi_d8.py:67-76: the dask+cupy branch and the plain dask branch now execute the same call (_twi_dask(fa_data, sl_data, cellsize)). Theis_dask_cupybranch can be removed so the genericisinstance(fa_data, da.Array)branch handles both, with the "purely elementwise, no host round-trip" comment moved there. One branch is harder to drift than two.
Nits (optional improvements)
-
xrspatial/hydro/watershed_d8.py:51-59: the memory-budget comment counts 33 B/px but the new init adds two transient boolean masks (about 2 B/px) plus thefd_valid & pp_validtemporary. Well inside the 50% guard headroom, so just a comment accuracy point. -
xrspatial/hydro/tests/test_twi_d8.py:198-216: the regression test readsresult.data._metaandHighLevelGraph.layers. Both are stable in practice but not public dask API; if a future dask release reworks graph internals this test will need a touch-up. A code comment already explains the intent, which helps.
What looks good
- Before/after timings are in the PR body and were measured on real hardware (twi dask+cupy 0.672 s to 0.033 s; watershed numpy 0.79 s to 0.104 s on 2000x2000).
- The vectorized init was checked for exact equality against the old loop output (
np.array_equal, NaN positions included) before the loop was removed, and a new test pins the one subtle semantic: a pour point on a NaN flow_dir cell stays nodata. - The graph-layer regression test compares layers added by twi rather than absolute graph size, so it is robust to differences in how the test inputs are constructed.
- The asv
TWIbenchmark gains cupy and dask+cupy params with aNotImplementedErrorguard in setup, matching the repo's convention for skipping GPU benchmarks on CPU-only hosts. - The dask+numpy paths and all tile-sweep drivers are untouched; the diff stays inside the two audited findings.
Checklist
- Algorithm matches reference/paper (no algorithm change; init semantics preserved exactly)
- All implemented backends produce consistent results (954 hydro tests pass on a CUDA host, cupy and dask+cupy lanes executed)
- NaN handling is correct (pour-point-on-NaN test added)
- Edge cases are covered by tests
- Dask chunk boundaries handled correctly (elementwise only; no overlap involved)
- No premature materialization or unnecessary copies (the PR removes one)
- Benchmark exists (TWI benchmark extended to the fixed path)
- README feature matrix updated: not applicable, tiers unchanged
- Docstrings present and accurate (
_twi_daskgained a one-liner)
brendancol
commented
Jul 23, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: follow-up pass after 38bf988
Re-checked the branch after the review follow-ups.
- Suggestion (redundant twi dask+cupy branch): fixed in 38bf988. The
is_dask_cupybranch is gone, the genericisinstance(fa_data, da.Array)branch handles both chunk backends, and the now-unusedis_dask_cupyimport was removed. Dispatch order stays safe:is_cupy_arrayis False for dask-wrapped cupy, so dask+cupy input still reaches_twi_dask. - Nit (memory-budget comment in watershed_d8): fixed in 38bf988; the comment now mentions the ~3 B/px transient masks and that they fit in the guard headroom.
- Nit (private dask API in the twi regression test): dismissed with reason. The
_meta/layersreads are deliberate, commented in the test, and there is no public API that exposes the same signal. Revisit only if a dask upgrade breaks it.
flake8 is clean on the touched files and the twi/watershed test files pass (47 tests). No new findings. Nothing blocking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3692
Two performance fixes in the hydro subpackage, from the 2026-07-23 /sweep-performance pass (OOM verdict RISKY unchanged, bottleneck memory-bound).
twi_d8dask+cupy: removed_twi_dask_cupy, which pulled every chunk to the host with.get(), ran the elementwise math on numpy, and pushed the result back withcp.asarray. The dask+cupy branch now calls_twi_daskdirectly;tan/where/logrun natively on cupy chunks. Measured on 8192x8192 with 2048-square chunks: 0.672 s before, 0.033 s after (20x). Output matches to 1.8e-15.watershed_d8/watershed_dinf/watershed_mfdand the cupy fallback paths of the dinf/mfd variants built theirlabels/stateinit arrays with nested Python loops over the full grid. Replaced withnp.whereexpressions that produce identical arrays. On 2000x2000 the loop was ~90% of the runtime;watershed_d8drops from 0.79 s to 0.104 s.Backend coverage: numpy and cupy paths of the watershed family, dask+cupy path of twi. The dask+numpy paths and all tile-sweep drivers are untouched.
Benchmarks: the asv
TWIbenchmark gainscupyanddask+cupyparams (skipped viaNotImplementedErrorwhen CUDA is unavailable), so a reintroduced host round-trip shows up as a regression. The existingWatershednumpy benchmark already covers the init path.Also includes the sweep-performance state CSV row update for the hydro module.
Test plan:
pytest xrspatial/hydro/tests/(954 passed, CUDA host, cupy and dask+cupy lanes executed)