Skip to content

Make hotspots() lazy for Dask input#2802

Open
brendancol wants to merge 2 commits into
mainfrom
issue-2772
Open

Make hotspots() lazy for Dask input#2802
brendancol wants to merge 2 commits into
mainfrom
issue-2772

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Summary

hotspots() computed the global mean and std eagerly with da.compute() while building the graph for Dask input, so calling it ran about 12 Dask tasks before you asked for any output (xrspatial/focal.py around lines 1289-1293, and the Dask+CuPy path around line 1336).

The Dask and Dask+CuPy paths are now lazy:

  • The global mean/std stay as 0-d dask reductions that broadcast into the z-score.
  • Each chunk is convolved via map_overlap, then classified via map_blocks.
  • The eager ZeroDivisionError is dropped on the dask path. The numpy and cupy single-array paths still raise it, and the zero-std test uses the numpy backend, so it stays green.

Backend coverage

  • numpy: unchanged
  • cupy: unchanged
  • dask+numpy: now lazy
  • dask+cupy: now lazy

Test plan

  • test_dask_laziness.py::test_hotspots_no_eager_compute asserts zero dask tasks run on the call (task-count callback), not just the return type.
  • Confirmed the dask result is bit-identical to the numpy result on a 30x40 random raster.
  • pytest xrspatial/tests/test_focal.py xrspatial/tests/test_emerging_hotspots.py xrspatial/tests/test_dask_laziness.py passes (218 passed).
  • flake8 clean on changed files.

Closes #2772

hotspots() used to call da.compute() on the global mean and std while
building the graph, so calling it ran about 12 Dask tasks before you
asked for any output. This keeps those reductions as lazy 0-d dask
arrays that broadcast into the z-score. The convolution runs through
map_overlap and the classification through map_blocks, so the Dask and
Dask+CuPy paths build their graph without running a single task.

test_dask_laziness.py now asserts zero tasks run on the call, via a
dask task-count callback, instead of only checking the return type.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 1, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Make hotspots() lazy for Dask input

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • focal.py: the convolve map_overlap passes meta=np.array((), dtype=np.float32), but for float64 input the convolved chunks are float64 (the up-front cast only runs for non-float input). The final result is still int8 from the map_blocks meta, and I verified correctness against numpy on float64 input, so this is cosmetic. If you want the intermediate meta to be exact you could cast the data to float32 unconditionally.

What looks good

  • The eager da.compute() is gone from both dask paths. The global mean/std are 0-d dask reductions broadcast into the z-score, so nothing runs on the call.
  • Convolution still goes through map_overlap with depth = kernel radius and the boundary forwarded from the public API, so halo handling is unchanged.
  • The new test asserts zero tasks execute on the call via a dask task-count callback, which is what actually catches this regression. The old eager path ran 12 tasks.
  • Dask output is bit-identical to numpy on float32, float64, integer, and ragged-chunk inputs.
  • The dask+cupy path mirrors the numpy path and keeps each chunk on the device.

Checklist

  • Algorithm matches reference (z-score classification unchanged)
  • All implemented backends produce consistent results
  • NaN handling is correct (boundary=nan halo unchanged)
  • Edge cases covered (ragged chunks, integer/float input verified)
  • Dask chunk boundaries handled correctly
  • No premature materialization
  • Benchmark exists or not needed (bug fix, no new API)
  • README feature matrix updated (not applicable)
  • Docstrings present and accurate

Address review nit: the convolve map_overlap declared a float32 meta but
float64 input stayed float64 through the intermediate. Cast to float32 up
front in both dask paths, matching the single-array numpy path.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review (after 29251b4)

The one nit from the previous pass is resolved. Both dask paths now cast the input to float32 up front, so the convolve map_overlap meta and the actual chunk dtype agree, and the dask paths line up with the single-array numpy path.

Rechecked after the change:

  • hotspots and dask-laziness tests pass (33 passed).
  • float64 input still produces a result bit-identical to numpy, lazy dtype int8.
  • flake8 clean.

No remaining blockers, suggestions, or nits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hotspots() computes global stats eagerly for Dask input instead of staying lazy

1 participant