Skip to content

Propagate NaN center cell in planar slope#2773

Merged
brendancol merged 1 commit into
mainfrom
issue-2761
Jun 1, 2026
Merged

Propagate NaN center cell in planar slope#2773
brendancol merged 1 commit into
mainfrom
issue-2761

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #2761

What

  • The planar _cpu and _gpu slope kernels read the 8 surrounding cells but never the center, so a NaN hole with valid neighbours returned a finite slope (a flat 5x5 raster with only the center NaN gave 0.0 at the center).
  • Add a center-cell NaN check to both kernels so a NaN center comes back as NaN, matching the geodesic method and normal NoData semantics.

Backend coverage

The check lives in the shared _cpu (@ngjit) and _gpu (cuda device) kernels, so it covers all four planar paths: numpy, cupy, dask+numpy, dask+cupy.

Test plan

  • test_center_nan_propagates_numpy - NaN center stays NaN
  • dask+numpy / cupy / dask+cupy variants compare against numpy with equal_nan=True
  • existing test_slope.py suite still green (65 passed locally, GPU included)

The planar _cpu and _gpu kernels read the 8 neighbours but not the
center cell, so a NaN hole surrounded by valid cells returned a finite
slope. Add a center-cell NaN check to both paths and pin it with
cross-backend regression tests.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 1, 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: Propagate NaN center cell in planar slope

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • xrspatial/slope.py:116 -- the CPU path checks the center with np.isnan and the GPU path uses math.isnan. Each is the right call for its kernel, so nothing to change here. Just flagging it so the split doesn't trip up a future reader.

What looks good

  • The fix lives in the two shared kernels (_cpu, _gpu), so all four planar backends pick up the behaviour without touching the dispatch wiring. numpy and cupy were verified locally on a real GPU; the dask paths run the same kernels through map_overlap.
  • continue (CPU) and return nan (GPU) both land on a NaN that out is already pre-filled with, so the center stays NaN and its neighbours go NaN on their own (they read the hole as one of their 8 inputs). Same result the geodesic method already gives.
  • Tests hit all four backends and check numpy-vs-backend parity with equal_nan=True, plus a direct assertion on the center cell. The regression comment names the issue.
  • Scope is tight. Geodesic paths and boundary handling are left alone.

Checklist

  • Algorithm matches reference: n/a (NoData propagation, not an algorithm change)
  • All implemented backends produce consistent results: yes
  • NaN handling is correct: yes
  • Edge cases covered by tests: yes (center NaN with valid neighbours, all four backends)
  • Dask chunk boundaries handled correctly: yes (depth=1 unchanged, kernel handles the NaN)
  • No premature materialization or unnecessary copies: yes
  • Benchmark exists or not needed: not needed (no perf change)
  • README feature matrix updated: n/a (no new function, no backend change)
  • Docstrings present and accurate: yes (public docstring unchanged and still valid)

@brendancol brendancol merged commit 0f080ad into main Jun 1, 2026
9 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.

Planar slope returns a valid value for a NoData (NaN) center cell

1 participant