viewshed: size max_distance window per axis on anisotropic rasters#2702
Merged
Conversation
_viewshed_windowed() computed a single cell radius from the coarser of ew_res/ns_res and applied it to both rows and columns. On anisotropic rasters this under-sized the window along the finer axis and clipped cells within max_distance to INVISIBLE. Use radius_rows from ns_res and radius_cols from ew_res. Adds a cross-backend regression test (numpy, cupy, dask).
brendancol
commented
May 29, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
PR Review: viewshed: size max_distance window per axis on anisotropic rasters
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
- xrspatial/tests/test_viewshed.py: the test only exercises one anisotropy direction (fine x, coarse y). A second case with the axes swapped (fine y, coarse x) would catch a future regression where someone wires radius_rows/radius_cols to the wrong resolution. Cheap to add via parametrize, but the current case already fails on the old code, so coverage of the bug itself is solid.
What looks good
- The fix is the right shape: radius_rows from ns_res, radius_cols from ew_res, each axis windowed by its own spacing.
- abs() on both resolutions handles north-up rasters where ns_res is negative.
- The post-window circular mask still uses real coordinates, so the larger window does not leak extra visible cells outside max_distance.
- The regression test checks values against the full viewshed (a real reference, not just "runs without error") and confirms out-of-range cells stay INVISIBLE.
- Runs across numpy, cupy (RTX), and dask; all go through the shared _viewshed_windowed path that was fixed.
Checklist
- Algorithm matches intended behavior (per-axis radius)
- All implemented backends produce consistent results
- NaN handling unchanged by this fix
- Edge cases: out-of-range cells, anisotropic spacing covered
- Dask path exercised (max_distance window)
- No premature materialization or unnecessary copies
- Benchmark not needed (clip-bound fix)
- README feature matrix unaffected (no API change)
- Docstring already documents max_distance accurately
brendancol
commented
May 29, 2026
Contributor
Author
brendancol
left a comment
There was a problem hiding this comment.
Follow-up review
The one nit from the previous pass is addressed: test_viewshed_max_distance_anisotropic is now parametrized over both anisotropy orientations (fine x / coarse y, and fine y / coarse x), so a swap between radius_rows and radius_cols would be caught. Full suite: 31 passed (numpy, cupy/RTX, dask).
No remaining Blockers, Suggestions, or Nits.
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 #2691
_viewshed_windowed()sized its analysis window from a single cell radius derived from the coarser ofew_res/ns_res, then used that radius for both rows and columns. On anisotropic rasters (different x and y spacing) the window came out too small along the finer axis, so cells withinmax_distancethere were dropped from the window and returned INVISIBLE.radius_rowsfromns_resandradius_colsfromew_resso each axis is windowed by its own resolution.Backends: the fix lives in
_viewshed_windowed(), the sharedmax_distancepath, so numpy, cupy, dask+numpy, and dask+cupy are all covered. The new test runs numpy, cupy (skipped without rtxpy), and dask.Test plan:
pytest xrspatial/tests/test_viewshed.py(28 passed, includes cupy/RTX and dask)