Document NaN propagation in edge_detection docstrings#3678
Open
brendancol wants to merge 2 commits into
Open
Conversation
brendancol
commented
Jul 18, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
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 withboundary='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_propagatesnow 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
commented
Jul 18, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
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.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3676
sobel_x,sobel_y,prewitt_x,prewitt_y, andlaplaciandocstrings stating that NaN input cells propagate to their 3x3 neighborhood, and that the defaultboundary='nan'leaves the outer one-cell ring NaN.test_nan_in_input_propagatesto assert the exact documented footprint: the 3x3 block around the NaN cell is NaN and nothing else is.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