Fix cupy backend crash in cumulative_viewshed/visibility_frequency#3205
Merged
Conversation
…3193) count was always initialized as a numpy array, so accumulating cupy viewshed results raised TypeError on the GPU backend. Initialize count as a cupy array when the raster is cupy-backed, mirroring the existing dask branch, so the result is a cupy-backed DataArray that carries the input coords, dims, and attrs. Add cupy cross-backend tests. Also records the visibility metadata sweep state.
brendancol
commented
Jun 10, 2026
brendancol
left a comment
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Fix cupy backend crash in cumulative_viewshed/visibility_frequency
Blockers
None.
Suggestions
None blocking. One thing worth a sentence in the docstring or a comment: cumulative_viewshed checks the dask branch before the cupy branch, so a dask-of-cupy raster goes through the dask path and never hits the new cp.zeros branch. That matches how the function already worked and the PR body calls it out, but a one-line comment at the branch would save the next reader the trace.
Nits
The new _is_cupy guard recomputes has_cuda_and_cupy(); that is cheap and matches the style of the surrounding code, so leaving it is fine.
What looks good
- The cupy branch mirrors the existing dask branch exactly: detect backend, allocate the accumulator on the right device, let the final DataArray wrap whatever
countis. Minimal and consistent. - Tests assert the result is a real
cupy.ndarray, check dtype, and confirm coords/dims/attrs (crs included) survive, which is the actual contract that was broken. - The single-observer test compares against the cupy
viewshedbinary mask rather than the numpy result, which is correct: the RTX viewshed and the CPU sweep disagree on visible-cell counts, so a numpy-vs-cupy value assertion would be wrong here. Good call.
Checklist
- Fix matches the backend-dispatch pattern used for dask
- cupy result is consistent with cupy viewshed
- coords/dims/attrs preserved on cupy
- Edge cases: empty observers still raises (unchanged path)
- No premature materialization introduced
- No new public API, so README/benchmark unaffected
- Tests gated on cuda+rtx availability
…isibility-2026-06-10
This was referenced Jun 11, 2026
…isibility-2026-06-10 Conflicts: - .claude/sweep-metadata-state.csv: took main's copy (LF line endings, newer rows for other modules) and re-inserted this branch's visibility row; diff vs main is exactly that one row. - xrspatial/visibility.py auto-merged: main's isort import ordering (#3189) composed with this branch's cupy accumulator change.
brendancol
added a commit
that referenced
this pull request
Jun 11, 2026
The two cupy tests pinned issue #3192 with xfail(strict=True), but open PR #3205 fixes that crash (filed there as #3193, a duplicate) and ships its own cupy parity tests for both functions. Merging the strict pins would break main with XPASS the moment #3205 lands, so they're dropped and the sweep-state note updated. The rest of the branch's coverage (cupy transect/line_of_sight parity, NaN LOS, Fresnel-blocked branch) is unaffected.
brendancol
added a commit
that referenced
this pull request
Jun 11, 2026
The dropped xfail pins are confirmed right: #3205's cupy fix for cumulative_viewshed/visibility_frequency is now on main with its own parity tests, which auto-merged cleanly alongside this branch's coverage. CSV keeps this branch's visibility row plus main's newer zonal row, with main's CRLF endings preserved.
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
1 participant
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 #3193
cumulative_viewshedinitialized its accumulatorcountas a numpy array for every non-dask raster, so accumulating cupyviewshedresults raisedTypeError: Unsupported type <class 'numpy.ndarray'>on the GPU backend.visibility_frequencywrapscumulative_viewshed, so it failed the same way.countas a cupy array when the raster is cupy-backed, mirroring the existing dask branch. The accumulation stays on-device and the result is a cupy-backed DataArray.Backend coverage: numpy and dask+numpy unchanged; cupy now works instead of crashing. dask+cupy isn't exercised by these functions (the dask branch runs before the cupy branch).
Test plan:
pytest xrspatial/tests/test_visibility.py(28 passed, including 3 new cupy tests on an RTX host)cupy.ndarraywith int32/float64 dtype and preserved coords/dims/attrscumulative_viewshedmatches the cupyviewshedbinary mask for a single observer