Skip to content

Skip full-raster geodesic memory guard for dask aspect#2774

Merged
brendancol merged 2 commits into
mainfrom
issue-2763
Jun 1, 2026
Merged

Skip full-raster geodesic memory guard for dask aspect#2774
brendancol merged 2 commits into
mainfrom
issue-2763

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2763

What changed

  • aspect(method='geodesic') no longer runs the full-raster memory guard for dask-backed inputs. The dask backends process the raster chunk-by-chunk via map_overlap, so peak memory is bounded by chunk size, not full-raster size. The guard now applies only to in-memory numpy/cupy arrays, where the whole (3, H, W) stack is materialized at once.

Backend coverage

  • numpy: unchanged, guard still runs
  • cupy: unchanged, guard still runs
  • dask+numpy: guard skipped (was incorrectly raising MemoryError)
  • dask+cupy: guard skipped (same path)

Test plan

  • New test: a 200x200 dask raster with (40, 40) chunks runs under geodesic aspect with available memory monkeypatched to 1 MB, and computes to the right shape. Verified this fails on unpatched code and passes after the fix.
  • Existing in-memory guard test still raises MemoryError when the full raster genuinely will not fit.
  • Full test_geodesic_aspect.py suite passes (36 tests).

The geodesic aspect path called _check_geodesic_memory against the full
raster before backend dispatch, even for dask arrays. The dask backends
process the raster chunk-by-chunk via map_overlap, so peak memory tracks
chunk size, not full-raster size. The guard now applies only to in-memory
numpy/cupy arrays. Adds a dask test (200x200, small chunks, low memory)
and keeps the in-memory guard test.
@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: Skip full-raster geodesic memory guard for dask aspect

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • The new test covers dask+numpy only. The dask+cupy path goes through the same is_dask branch in aspect.py, so the fix applies to it too, but there is no test asserting dask+cupy skips the guard under low memory. A small @cuda_and_cupy_available test mirroring the dask+numpy one would lock that path down. Not blocking, since the code path is shared and the guard skip is backend-agnostic.

Nits (optional improvements)

  • The same full-raster guard pattern exists in slope.py:397 with the identical dask-rejection bug. Out of scope for this PR (the issue is scoped to aspect), but worth a follow-up so slope's dask geodesic path isn't blocked the same way.

What looks good

  • The is_dask check reuses the project's standard detection (has_dask_array() and isinstance(agg.data, da.Array)), matching ArrayTypeFunctionMapping and validate_arrays.
  • The guard still runs for numpy/cupy, so the in-memory OOM protection is intact. The existing oversized-raster test confirms it.
  • The comment explains why dask is exempt (chunk-by-chunk via map_overlap), so the next reader won't re-add the guard.
  • The new test forces .compute(), so it actually exercises the chunked path rather than just building a lazy graph.

Checklist

  • Algorithm matches reference/paper: n/a, no algorithm change
  • All implemented backends produce consistent results: unchanged
  • NaN handling is correct: unchanged
  • Edge cases covered by tests: guard-skip for dask and guard-raise for in-memory both tested
  • Dask chunk boundaries handled correctly: unchanged map_overlap path
  • No premature materialization or unnecessary copies: confirmed, the fix removes a false rejection only
  • Benchmark exists or not needed: not needed
  • README feature matrix updated: not applicable, no API or backend change
  • Docstrings present and accurate: unchanged public docstring still accurate

Addresses review suggestion: the dask+cupy path shares the is_dask branch
but had no test asserting it skips the full-raster guard under low memory.
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 204a20c)

Disposition of the prior findings:

  • Suggestion (dask+cupy guard-skip test): fixed. Added test_dask_cupy_chunked_skips_full_raster_guard, gated on cuda_and_cupy_available, mirroring the dask+numpy test.
  • Nit (slope.py:397 has the same dask-rejection bug): deferred to #2778. It's a separate module outside this issue's scope and warrants its own change with its own slope tests.

No new findings on the added test. The memory-guard tests pass (the dask+cupy one skips here without a GPU, as expected). Nothing else changed.

@brendancol brendancol merged commit 4ea139e into main Jun 1, 2026
6 of 7 checks passed
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.

Chunked geodesic dask aspect blocked by full-raster memory guard

1 participant