Skip to content

polygonize: reject non-finite simplify_tolerance (nan, inf)#2584

Merged
brendancol merged 6 commits into
mainfrom
issue-2575
May 28, 2026
Merged

polygonize: reject non-finite simplify_tolerance (nan, inf)#2584
brendancol merged 6 commits into
mainfrom
issue-2575

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Summary

  • Reject non-finite values (nan, +inf, -inf) for polygonize's simplify_tolerance argument with a clear ValueError. Previously nan silently disabled simplification (because nan > 0 is False) and +inf collapsed every polygon to empty output.
  • Mirrors the existing atol / rtol validation contract in the same function: finite and non-negative.

Closes #2575

Backend coverage

Validation lives in the public polygonize entry point, before backend dispatch, so the new check applies uniformly to numpy, cupy, dask+numpy, and dask+cupy.

Test plan

  • simplify_tolerance=nan raises ValueError
  • simplify_tolerance=+inf raises ValueError
  • simplify_tolerance=-inf raises ValueError (was rejected by sign, now by finite check too)
  • simplify_tolerance=-1.0 still raises (regression)
  • simplify_tolerance=0.0 and simplify_tolerance=1e-9 still succeed (guard against over-eager fix)
  • Full TestPolygonizeSimplify class passes locally (18 passed, 1 skipped)

Mirror the atol/rtol validation already used in the same function:
require simplify_tolerance to be finite and non-negative.

Previously only negative values were rejected. NaN silently disabled
simplification because the gating check `simplify_tolerance > 0` is
False for NaN, and +inf collapsed every polygon to empty output.
Both now raise ValueError with a clear message.

Tests cover nan, +inf, -inf (regression for sign-only rejection of
-inf), the existing negative case, and 0.0 / small-positive happy
paths so an over-eager fix that bans non-positive values would also
fail.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 28, 2026

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: polygonize: reject non-finite simplify_tolerance (nan, inf)

Blockers (must fix before merge)

  • None.

Suggestions (should fix, not blocking)

  • None.

Nits (optional improvements)

  • None. The simplify_tolerance validation could be co-located with the atol/rtol validation block at xrspatial/polygonize.py:2013 for tighter grouping, but the current placement keeps it next to simplify_method validation and matches the pre-existing structure. Moving it would be churn.

What looks good

  • The check not np.isfinite(simplify_tolerance) or simplify_tolerance < 0 is correct: np.isfinite returns False for nan / +inf / -inf and True for normal finite floats including 0. Combined with the existing < 0 guard, this allows only finite non-negative values (which includes 0.0). Python or short-circuits, so the < 0 comparison never sees nan and we avoid the RuntimeWarning: invalid value encountered in less that bare numpy comparisons would emit.
  • Validation lives in the public polygonize entry point before ArrayTypeFunctionMapping dispatch, so the new contract applies uniformly to numpy, cupy, dask+numpy, and dask+cupy without per-backend duplication.
  • Error message matches the wording style of the existing atol/rtol errors ("must be a non-negative finite number, got ..."), and match="simplify_tolerance" in the tests stays stable across future message tweaks.
  • Tests use a uniform 2x2 raster for the failure cases (no actual polygon edges) and a 2x4 two-label raster for the happy-path 0.0 / 1e-9 case so simplification has something to act on. The split is appropriate -- validation runs before geometry work, so the failure-case raster does not need real edges.
  • The test_zero_and_small_positive_tolerance_still_work regression guard is a nice touch: it would catch a future "fix" that bans non-positive along with non-finite.
  • CHANGELOG entry under Unreleased / Fixed is accurate and references the issue.

Checklist

  • Algorithm matches reference/paper -- N/A, input validation only
  • All implemented backends produce consistent results -- validation runs pre-dispatch
  • NaN handling is correct -- nan rejected up front, no downstream NaN propagation concerns
  • Edge cases are covered by tests -- nan, +inf, -inf, -1.0, 0.0, 1e-9
  • Dask chunk boundaries handled correctly -- N/A, no chunk-level code touched
  • No premature materialization or unnecessary copies -- N/A, scalar validation only
  • Benchmark exists or is not needed -- not needed for a one-line validation check
  • README feature matrix updated (if applicable) -- N/A, no API surface change
  • Docstrings present and accurate -- existing simplify_tolerance docstring at line 1877 still applies; consistent with atol/rtol which also do not enumerate their validation in the docstring

@brendancol brendancol merged commit 97bc7ca into main May 28, 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.

polygonize: reject non-finite simplify_tolerance (nan, inf)

1 participant