Skip to content

Validate planar cell size in slope() (#2897)#2901

Merged
brendancol merged 5 commits into
mainfrom
issue-2897
Jun 3, 2026
Merged

Validate planar cell size in slope() (#2897)#2901
brendancol merged 5 commits into
mainfrom
issue-2897

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Summary

  • Add cell-size validation to the planar slope() path, matching the guard already in curvature().
  • slope(method='planar') now raises a clear ValueError when the resolution on either axis is zero, negative, NaN, or infinite. Before this, zero gave a raw ZeroDivisionError, NaN gave a silently-NaN interior, and inf gave wrong values that looked plausible.
  • The check sits in the public wrapper before backend dispatch, so it behaves the same on numpy, cupy, dask+numpy, and dask+cupy. The geodesic path is untouched since it takes its scale from lat/lon coordinates, not the res attribute.

Backend coverage

numpy, cupy, dask+numpy, dask+cupy (planar). Validation runs in the wrapper, so all four behave identically.

Test plan

  • Parametrized tests assert the ValueError for zero/negative/NaN/inf cell sizes across all four backends.
  • Regression test confirms a normal positive cell size still works.
  • Test confirms the geodesic path ignores a bogus res attribute and does not trip the planar guard.
  • test_slope.py and test_geodesic_slope.py pass (153 tests).

Closes #2897

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 3, 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: Validate planar cell size in slope() (#2897)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • slope.py:406-414 The guard uses cellsize_x <= 0 while the sibling guard in curvature.py:256-263 uses cellsize_x == 0. The divergence is deliberate: issue #2897 asks slope to reject negative resolutions too, and a negative cell size would silently flip the slope sign. A one-line comment would stop a future reader from "harmonizing" the two guards back to == 0.
  • test_slope.py:483-520 The four backend-specific parametrized tests are near-identical. Validation runs in the wrapper before dispatch, so the numpy test alone already covers the code path. The cupy/dask variants are cheap insurance that no backend swallows the error, so keeping them is fine; they could be collapsed if the file grows.

What looks good

  • The check sits in the public wrapper before ArrayTypeFunctionMapping dispatch, so all four backends behave the same without per-backend code. Matches the curvature.py precedent.
  • NaN/inf ordering is correct: not np.isfinite(...) is evaluated first via or, so NaN never reaches the <= 0 comparison (which is False for NaN and would let it through).
  • The geodesic path is left untouched, with an explicit test confirming a bogus res attribute does not trip the planar guard.
  • The error message names the offending values and says how to fix the resolution, in the curvature.py message style.
  • Tests hit zero, negative, NaN, and inf on each axis independently, plus a positive-cellsize regression test.

Checklist

  • Algorithm matches reference/paper (no algorithm change; validation only)
  • All implemented backends produce consistent results (validation is backend-agnostic, runs in wrapper)
  • NaN handling is correct
  • Edge cases are covered by tests
  • Dask chunk boundaries handled correctly (unchanged; guard runs before dispatch)
  • No premature materialization or unnecessary copies
  • Benchmark exists or is not needed (validation guard, no perf impact)
  • README feature matrix updated (not applicable; no new function or backend change)
  • Docstrings present and accurate

@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.

Follow-up review (after f1a5eab)

The single actionable nit from the previous pass is resolved: slope.py now carries a one-line comment explaining why the guard uses <= 0 (rejecting negatives) where curvature() uses == 0. No new code paths, so the earlier checklist still holds.

The second nit (collapsing the four near-identical backend tests) is left as-is on purpose. The cupy and dask variants are cheap and confirm no backend wrapper swallows the error before it reaches the user. Collapsing them would trade that coverage for a few saved lines, which is not worth it here.

No remaining blockers or suggestions.

@brendancol brendancol merged commit 911d95c into main Jun 3, 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.

slope() planar method does not validate cell size

1 participant