Skip to content

Validate viewshed max_distance (reject negative / non-finite)#2874

Merged
brendancol merged 4 commits into
mainfrom
issue-2855
Jun 3, 2026
Merged

Validate viewshed max_distance (reject negative / non-finite)#2874
brendancol merged 4 commits into
mainfrom
issue-2855

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2855

viewshed() didn't validate max_distance. Negative, NaN, and infinite
values fell through to the windowing math and raised internal errors that
never mentioned the argument (for example "zero-size array to reduction
operation minimum" for -1.0 and "cannot convert float NaN to integer" for
NaN).

What changed:

  • Validate max_distance at the public viewshed() entry point, before
    backend dispatch, and raise ValueError("max_distance must be a finite number >= 0, ...") for negative or non-finite values.
  • Document the requirement on the max_distance parameter.

Backend coverage: the check runs before dispatch, so it applies to all
four backends (numpy, cupy, dask+numpy, dask+cupy).

Test plan:

  • New test asserts the clear ValueError for -1.0, -0.5, NaN, +inf, -inf.
  • New test confirms finite values (0.0, 3.0) still work on numpy and dask+numpy.
  • Full test_viewshed.py passes (72 passed, 1 xfailed).

Skipped the user-guide-notebook and README-matrix steps: this is a pure
input-validation bug fix with no new public API and no change to backend
support.

…#2855)

A negative, NaN, or infinite max_distance fell through the windowing
math and raised confusing internal errors that never mentioned the
argument. Validate at the public viewshed() entry point, before backend
dispatch, so all four backends are covered, and raise a clear ValueError.

Add tests for the rejected values and confirm finite values >= 0 still
work.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 2, 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: Validate viewshed max_distance (reject negative / non-finite)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • xrspatial/viewshed.py:1721 and 1729: there are now two back-to-back if max_distance is not None: blocks. You could fold the validation into the existing block, but keeping them separate reads fine too (validation first, windowing dispatch under its own comment). Optional.

What looks good

  • Validation sits at the public entry point before backend dispatch, so all four backends (numpy, cupy, dask+numpy, dask+cupy) are covered by one check. The test docstring spells out that reasoning.
  • The error message names the argument and echoes the bad value, which is the point of the fix.
  • The try/except (TypeError, ValueError) also turns a non-numeric max_distance into the same clear message instead of a raw np.isfinite TypeError.
  • Tests cover -1.0, -0.5, NaN, +inf, -inf and confirm 0.0 and 3.0 still work on numpy and dask.
  • Docstring updated to state the contract.

Checklist

  • Validation runs before dispatch, so all backends behave the same
  • NaN / inf / negative handled with a clear ValueError
  • Edge cases (0.0, +inf, -inf, non-numeric) covered
  • No new materialization or copies
  • No new public API, so README matrix and user guide are not needed
  • Docstring present and accurate

Address review nit: merge the two consecutive 'if max_distance is not
None' blocks so the check and the windowed dispatch share one branch.
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 (after 9104c38)

The one nit from the first pass is resolved: the validation and the windowed dispatch now share a single if max_distance is not None: branch (xrspatial/viewshed.py:1721). Behavior is unchanged and the 24 max_distance tests still pass.

No remaining Blockers, Suggestions, or Nits.

@brendancol brendancol merged commit 23a869b into main Jun 3, 2026
9 of 10 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 is not validated (negative/NaN/inf give confusing internal errors)

1 participant