Add Examples sections and fix cross-correlation wording in edge_detection docstrings#3683
Open
brendancol wants to merge 3 commits into
Open
Add Examples sections and fix cross-correlation wording in edge_detection docstrings#3683brendancol wants to merge 3 commits into
brendancol wants to merge 3 commits into
Conversation
brendancol
commented
Jul 18, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Add Examples sections and fix cross-correlation wording in edge_detection docstrings
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
- Each gradient docstring now says "cross-correlating" twice: once in the lead sentence ("Detects vertical edges by cross-correlating with...") and again in the paragraph right after the kernel ("The kernel is applied by cross-correlation, as in
scipy.ndimage.correlate..."), e.g. xrspatial/edge_detection.py:34 and :39. The second paragraph earns its keep by naming the scipy equivalent and the sign convention, but the two could be merged into one sentence if you want the docstrings tighter. - Coordination, not a defect: #3678 inserts Notes sections into these same five docstrings right after Returns, where this PR inserts Examples. Whichever merges second gets a conflict at every insertion point. The PR body already says this; resolution is mechanical (numpydoc order puts Notes before Examples).
Nits (optional improvements)
-
laplacian's docstring skips the cross-correlation paragraph. Correct, since the kernel is symmetric and the two operations agree, but a reader comparing the five docstrings may wonder why it's absent. A short clause ("the kernel is symmetric, so correlation and convolution agree") would preempt that. Fine to leave as is.
What looks good
- All five examples are real output: they pass bare
doctest.testmod(31 examples) andpytest --doctest-moduleson this host. - The sign-convention fix is backed by measurement, not reading: sobel_x on a left-to-right ramp returns +8, matching
scipy.ndimage.correlate, while convolving the documented kernel gives -8. The old wording predicted the wrong sign. - The drift-guard tests follow the existing patterns (
test_docstring_params_match_signaturein test_multispectral.py, phrase-pinning as in test_proximity.py) and pin "cross-correlat" / reject "by convolving" so the wording can't silently regress. - Doc-only: no function body changed, and the full test file passes (101 tests). Flake8 findings are unchanged from baseline (4 E201 in the kernel literals, 1 E302 in the test file, all pre-existing).
Checklist
- Algorithm matches reference/paper (wording now matches the measured correlate semantics)
- All implemented backends produce consistent results (verified numpy/cupy/dask+numpy/dask+cupy during the audit; unchanged by this PR)
- NaN handling is correct (unchanged; NaN docs land in #3678)
- Edge cases are covered by tests (unchanged coverage plus new docstring-contract tests)
- Dask chunk boundaries handled correctly (n/a, doc-only)
- No premature materialization or unnecessary copies (n/a, doc-only)
- Benchmark exists or is not needed (not needed, doc-only)
- README feature matrix updated (n/a, no new functions or backend changes)
- Docstrings present and accurate
brendancol
commented
Jul 18, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review follow-up (after 90592c2)
Disposition of the first review's findings:
- Duplicated "cross-correlating"/"cross-correlation" wording: fixed. The paragraph after each gradient kernel now reads "This matches
scipy.ndimage.correlate: the response is positive where values increase toward higher column/row index", so the operation is named once, in the lead sentence. - Overlap with #3678: no change in this PR, by design. Whichever PR merges second resolves the insertion-point conflict; numpydoc order puts its Notes above these Examples.
laplaciansymmetry nit: fixed. Its docstring now says "The kernel is symmetric, so cross-correlation and convolution agree."
Re-verified after the commit: pytest --doctest-modules on the module plus the test file passes (106 tests, includes all 31 docstring examples), and flake8 shows only the pre-existing baseline findings. No further issues.
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 #3674
sobel_x,sobel_y,prewitt_x,prewitt_y,laplacian). The module had no>>>blocks at all; every example here was executed and is doctest-verified.scipy.ndimage.correlatesemantics) and states the sign convention. The old "convolving with" wording predicted the negated result for the antisymmetric Sobel/Prewitt kernels: on a left-to-right ramp,sobel_xreturns +8 where convolving the documented kernel gives -8. The code matches the standard Sobel/Prewitt convention, so only the wording changes.test_docstring_params_match_signatureandtest_docstring_states_all_backends: documented parameters must match the signatures, Examples sections must exist, and the cross-correlation wording is pinned.Doc-only; no behavior changes. Also updates the documentation sweep state CSV.
Overlap note: #3678 (error-handling sweep) documents NaN propagation in these same docstrings via Notes sections. I dropped my NaN wording to stay out of its way, but both PRs insert text right after Returns, so whichever merges second will need a trivial conflict resolution (numpydoc order puts Notes before Examples).
Test plan:
python -m docteston the module: 31 examples passpytest xrspatial/tests/test_edge_detection.py: 101 passed