Skip to content

Document NaN propagation in edge_detection docstrings#3678

Open
brendancol wants to merge 2 commits into
mainfrom
deep-sweep-error-handling-edge_detection-2026-07-18
Open

Document NaN propagation in edge_detection docstrings#3678
brendancol wants to merge 2 commits into
mainfrom
deep-sweep-error-handling-edge_detection-2026-07-18

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3676

  • Adds a Notes section to the sobel_x, sobel_y, prewitt_x, prewitt_y, and laplacian docstrings stating that NaN input cells propagate to their 3x3 neighborhood, and that the default boundary='nan' leaves the outer one-cell ring NaN.
  • Tightens test_nan_in_input_propagates to assert the exact documented footprint: the 3x3 block around the NaN cell is NaN and nothing else is.
  • Also commits the error-handling sweep's state CSV row for this module.

Behavior is unchanged; this documents what the code already does. The footprint is identical on numpy, cupy, dask+numpy, and dask+cupy (checked on a GPU host).

Test plan:

  • pytest xrspatial/tests/test_edge_detection.py - 87 passed, including the cupy and dask+cupy classes

@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: Document NaN propagation in edge_detection docstrings

Blockers (must fix before merge)

  • none

Suggestions (should fix, not blocking)

  • none

Nits (optional improvements)

  • xrspatial/edge_detection.py (Notes sections, all five functions): "every output cell whose 3x3 neighborhood contains a NaN becomes NaN" is exact for interior NaNs, but under boundary='wrap' a NaN on one edge also poisons cells on the opposite edge (checked: NaN at (2,0) on a 5x6 raster with boundary='wrap' makes rows 1-3 of the last column NaN, since the wrapped neighborhood crosses the seam). Wording like "3x3 neighborhood (as extended by the boundary mode)" would cover that case.

What looks good

  • The documented claim was verified against observed behavior, not just read from the code: one interior NaN produces exactly a 3x3 NaN block, identical on numpy, cupy, dask+numpy, and dask+cupy.
  • test_nan_in_input_propagates now asserts the full footprint (NaN block and everything else non-NaN) instead of a single cell, so the docstring claim is pinned by a test.
  • Behavior is unchanged; the diff is docstrings, one test, and the sweep's state CSV row, which belongs in this PR per the sweep contract.

Checklist

  • Algorithm matches reference/paper (n/a, docs-only)
  • All implemented backends produce consistent results (footprint checked on all four)
  • NaN handling is correct (and now documented)
  • Edge cases are covered by tests
  • Dask chunk boundaries handled correctly (n/a, no code change)
  • No premature materialization or unnecessary copies (n/a)
  • Benchmark exists or is not needed (docs-only)
  • README feature matrix updated (n/a, no new functions or backend changes)
  • Docstrings present and accurate (see nit on wrap wording)

@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: follow-up after 907f8ed

The single nit from the first pass is addressed: all five Notes sections now read "3x3 neighborhood (as extended by the boundary mode)", which covers the wrap-seam case observed during review. Re-ran pytest xrspatial/tests/test_edge_detection.py: 87 passed.

No blockers, no suggestions, no nits remaining.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

edge_detection docstrings do not mention NaN propagation

1 participant