Skip to content

Add Examples sections and fix cross-correlation wording in edge_detection docstrings#3683

Open
brendancol wants to merge 3 commits into
mainfrom
deep-sweep-documentation-edge_detection-2026-07-18
Open

Add Examples sections and fix cross-correlation wording in edge_detection docstrings#3683
brendancol wants to merge 3 commits into
mainfrom
deep-sweep-documentation-edge_detection-2026-07-18

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3674

  • Adds an Examples section to each of the five public functions (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.
  • Rewords the four gradient docstrings to say the kernel is applied by cross-correlation (scipy.ndimage.correlate semantics) 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_x returns +8 where convolving the documented kernel gives -8. The code matches the standard Sobel/Prewitt convention, so only the wording changes.
  • Adds drift-guard tests modeled on test_docstring_params_match_signature and test_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 doctest on the module: 31 examples pass
  • pytest xrspatial/tests/test_edge_detection.py: 101 passed
  • flake8: no new findings (the 4 E201s in the kernel literals and the E302 in the test file predate this PR)

@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: 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) and pytest --doctest-modules on 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_signature in 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 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 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.
  • laplacian symmetry 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.

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: no examples, convolution wording implies the wrong sign, NaN propagation undocumented

1 participant