Skip to content

viewshed: size max_distance window per axis on anisotropic rasters#2702

Merged
brendancol merged 4 commits into
mainfrom
deep-sweep-accuracy-viewshed-2026-05-29
May 30, 2026
Merged

viewshed: size max_distance window per axis on anisotropic rasters#2702
brendancol merged 4 commits into
mainfrom
deep-sweep-accuracy-viewshed-2026-05-29

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2691

_viewshed_windowed() sized its analysis window from a single cell radius derived from the coarser of ew_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 within max_distance there were dropped from the window and returned INVISIBLE.

  • Compute radius_rows from ns_res and radius_cols from ew_res so each axis is windowed by its own resolution.
  • Add a cross-backend regression test on an anisotropic raster (ew_res=1, ns_res=10) that checks in-range cells along the finer axis match the full viewshed instead of being clipped.

Backends: the fix lives in _viewshed_windowed(), the shared max_distance path, 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)
  • Reproducer from the issue now matches the full viewshed within the radius

_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).
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 29, 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: 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

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

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.

@brendancol brendancol merged commit fb67463 into main May 30, 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.

viewshed max_distance clips cells on anisotropic rasters

1 participant