Skip to content

Warn on non-atomic custom GPU merge over overlap - #3061

Merged
brendancol merged 2 commits into
mainfrom
issue-3057
Jun 9, 2026
Merged

Warn on non-atomic custom GPU merge over overlap#3061
brendancol merged 2 commits into
mainfrom
issue-3057

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3057

The GPU backends only use CUDA atomics for the six built-in merges. A custom callable merge falls into the non-atomic read-modify-write path in _apply_merge_gpu, so overlapping geometries race and the overlap pixels are nondeterministic. The public docstring said nothing about this.

  • Added a .. warning:: to the merge docstring explaining that a custom GPU merge is nondeterministic over overlap, and that the built-in string merges stay deterministic because they use atomics.
  • Emit a UserWarning when a callable merge is combined with use_cuda=True (covers both cupy and dask+cupy). The warning fires up front, before the geometry burn and the CuPy check.

No behavior change for existing results; the GPU callable path is unchanged.

Backends

  • numpy / dask+numpy: unaffected (CPU writes each pixel single-threaded; no warning).
  • cupy / dask+cupy: warning now fires for callable merges.

Test plan

  • New tests in test_rasterize_gpu_callable_warn_3057.py: warning fires for callable + use_cuda=True and for the chunks (dask+cupy) variant; no warning for a CPU callable merge or for a built-in GPU merge. They record warnings manually so they pass without a GPU.
  • Existing test_rasterize.py suite still passes (215 passed, 2 skipped).
  • flake8 clean on the changed files.

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 9, 2026

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR Review: Warn on non-atomic custom GPU merge over overlap

Blockers

None.

Suggestions

None blocking. One thing worth weighing (see nits).

Nits (optional)

  • xrspatial/rasterize.py (~line 3181): the warning fires before the cupy is None ImportError guard at ~line 3413. A caller with no CuPy who passes a callable merge plus use_cuda=True gets the UserWarning first, then the ImportError. The ordering is harmless since the warning is true either way, but if you'd rather a missing-dependency caller only see the hard error, move the warn after the cupy check. Not a blocker.
  • test_rasterize_gpu_callable_warn_3057.py (_overlap_warnings): the bare except Exception: pass is intentional and documented (no GPU in CI), but it would also swallow a real regression where rasterize raised before the warn line. The len(matched) == 1 assert still catches the "warning never fired" case, so coverage holds. Flagging only so it stays a conscious choice.

What looks good

  • Warning sits in the single callable-merge branch, so it covers both cupy and dask+cupy without duplicated logic.
  • The docstring warning names the deterministic built-in merges, so the user gets an actionable alternative.
  • Tests check both directions: callable+GPU warns; CPU callable and built-in GPU merge stay silent. They run without a GPU by recording warnings manually.
  • No behavior change to the existing GPU callable path; the existing rasterize suite stays green.

Checklist

  • Algorithm matches reference: n/a (doc + warning only)
  • All implemented backends consistent: unchanged
  • NaN handling correct: unchanged
  • Edge cases covered by tests: yes (CPU vs GPU, callable vs string)
  • Dask chunk boundaries handled: unchanged; chunks path has a warning test
  • No premature materialization: n/a
  • Benchmark exists or not needed: not needed (no perf change)
  • README feature matrix updated: not needed (no new function, no backend change)
  • Docstrings present and accurate: yes

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up review (after fixes)

Both nits from the first pass are addressed.

Resolved

  • Warning now fires inside the else (callable) branch after the cupy is None ImportError check (xrspatial/rasterize.py ~line 3420). A caller with no CuPy now gets only the ImportError, not a UserWarning they can't act on. Still covers both the cupy and dask+cupy paths since gpu_merge_name is resolved there for both.
  • The two GPU warning tests are gated with @skip_no_cupy, matching the new ordering. The CPU "does not warn" test completes without a GPU, so it no longer relies on the broad except to pass. The remaining except Exception only swallows the device-less kernel-launch error, and the exact-count asserts still fail loudly if the warning never fires.

Remaining

None. No blockers, no open suggestions.

Verification

  • New file test_rasterize_gpu_callable_warn_3057.py: 4 passed.
  • test_rasterize.py: 215 passed, 2 skipped.
  • flake8 clean on both changed files.

@brendancol
brendancol merged commit 2731bc1 into main Jun 9, 2026
7 checks passed
@brendancol
brendancol deleted the issue-3057 branch June 25, 2026 12:58
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.

rasterize: custom CUDA merge functions are documented as supported but race on overlap

1 participant