Validate viewshed scalar params before dispatch (#2794)#3037
Merged
Conversation
viewshed() now rejects NaN, inf, and non-numeric observer_elev or target_elev with a ValueError naming the bad parameter, raised at the public entry point before backend dispatch. Same guard style as the existing max_distance check and curvature.py, so every backend gets the clear error instead of a confusing downstream failure. Tests cover the rejected values, the before-dispatch behavior on a dask raster, and that finite (including negative) elevations still work.
brendancol
commented
Jun 8, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Validate viewshed scalar params before dispatch (#2794)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
xrspatial/viewshed.py(_validate_finite_scalar): the new helper repeats the inline try/except pattern already used formax_distancefurther down inviewshed(). Not worth refactoring here sincemax_distancecarries an extra>= 0rule and a different message, but a future cleanup could routemax_distancethrough the same helper plus a range check.
What looks good
- Validation sits at the public entry point before any backend dispatch, so a single guard covers numpy, cupy, dask+numpy, and dask+cupy. The dask test confirms it raises up front rather than from inside the backend.
- The helper rejects NaN, inf, None, lists, and numpy arrays correctly (
not np.isfinite(arr)on an array raises, and the except clause catches it). A bool likeTruepasses, which is fine for a numeric scalar. - Tests cover the rejected values, before-dispatch behavior on a dask raster, and that finite elevations including negative still return a result.
- Docstring updated for both parameters.
Checklist
- NaN handling is correct
- Edge cases covered by tests
- All backends covered (entry-point validation, backend-agnostic)
- No premature materialization
- Benchmark not needed (validation only)
- README not applicable (no new function or backend change)
- Docstrings present and accurate
# Conflicts: # xrspatial/viewshed.py
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 #2794
What
viewshed()now validatesobserver_elevandtarget_elevat the public entry point, before any backend dispatch. NaN, inf, and non-numeric values raise aValueErrorthat names the offending parameter (e.g.observer_elev must be a finite number, got nan).The
max_distance >= 0/ finite guard already landed onmain; this PR adds the matching elevation checks alongside it so all the scalar validation happens up front. The guard style follows the existingmax_distancecheck and the cell-size check incurvature.py.Backends
Entry-point validation, backend-agnostic. Verified on numpy and dask+numpy; cupy/dask+cupy take the same path (no GPU available in this environment).
Test plan
observer_elevandtarget_elevraiseValueErrornaming the parametermax_distancevalidation tests still passtest_viewshed.pysuite passes (103 passed, 1 GPU skip)