convolution: document the cross-correlation convention in convolve_2d and pin it with a test#3619
Merged
brendancol merged 3 commits intoJul 6, 2026
Conversation
… pin it with a test convolve_2d applies the kernel without flipping it, so it computes cross-correlation, not a true convolution. Verified against scipy.ndimage 1.16.1: interior pixels match scipy.ndimage.correlate exactly across all boundary modes and differ from scipy.ndimage.convolve on an asymmetric kernel. The docstring called this a convolution without stating the kernel is not flipped. Add a note to the convolution_2d docstring stating the convention, and a golden test pinning the correlation semantics. reference-validation sweep 2026-07-02
brendancol
commented
Jul 6, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: convolution: document the cross-correlation convention in convolve_2d and pin it with a test
Doc-and-test PR. I verified the underlying claim rather than taking it on faith, and it holds.
Verification
- Read
_convolve_2d_numpy(convolution.py:348). The kernel index isiii = wkx + ii - i, which advances in the same direction as the data offset, so the kernel is applied as-is (not flipped). That is cross-correlation, matching the docstring note. The CUDA path comment at convolution.py:431 (kernel[k, h] * data[i + k - delta_rows, ...]) uses the same convention, so the backends agree. - Ran the new golden test plus a direct scipy 1.16.1 cross-check: interior pixels equal
scipy.ndimage.correlateand differ fromscipy.ndimage.convolvefor the asymmetric kernel used. The chosen kernel is genuinely asymmetric under 180-degree rotation, so it does distinguish the two. - Full
test_convolution.pypasses (7 tests).
Blockers
None.
Suggestions
None blocking.
Nits
- The golden test pins absolute values on the numpy backend only. That is the right call for a value-pinning test, and cross-backend consistency of the same convention is already covered by the parity tests in
test_focal.py(custom/circle/annulus kernels across numpy/dask/cupy), so a CUDA-only kernel flip would still be caught there. Worth being aware the two tests are complementary rather than redundant. - The docstring note lives on
convolution_2d, the fully documented public function;convolve_2d(convolution.py:480) is the array-level entry point and carries no docstring, so there is nowhere natural to repeat the note there. Fine as is.
What looks good
- The note names the concrete escape hatch (
kernel[::-1, ::-1]forscipy.ndimage.convolvesemantics) instead of just stating the convention, which is what a user hitting the mismatch actually needs. - The test comment records the reference tool and version and states its intent (guard against a silent kernel flip on refactor), so the golden numbers are reproducible.
- Scope is tight: a docstring paragraph, one test, and the sweep state CSV. No behavior change.
Checklist
- Algorithm matches reference (scipy.ndimage.correlate, verified)
- Backend convention consistent (numpy/CUDA source agree; parity via test_focal.py)
- NaN handling unchanged (no behavior change)
- Edge cases: correlation-vs-convolution distinction tested with an asymmetric kernel
- No premature materialization or copies (docs/tests only)
- Benchmark not needed (no behavior/perf change)
- README feature matrix: not applicable (no new function)
- Docstring present and accurate
…validation-convolution-2026-07-02 # Conflicts: # .claude/sweep-reference-validation-state.csv
brendancol
added a commit
to brendancol/xarray-spatial
that referenced
this pull request
Jul 6, 2026
…y-contrib#3616/xarray-contrib#3619/xarray-contrib#3620 Keep both sides in convolution.py (_validate_kernel from this branch, _PARALLEL_KERNEL_LOCK + parallel=True kernel from main) and both test blocks. State CSV: main's file with this branch's convolution row.
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.
convolve_2d applies the kernel as-is, without flipping it, so it computes cross-correlation and not a true convolution. I checked it against scipy.ndimage 1.16.1: interior pixels match
scipy.ndimage.correlateexactly (max abs diff 0.0) under all four boundary modes, and they differ fromscipy.ndimage.convolveonce the kernel is asymmetric.The docstring just called this a "2D convolution" and never mentioned the kernel isn't flipped. Anyone arriving from
scipy.ndimage.convolvewith an asymmetric kernel gets different numbers than they'd expect. The built-incircle_kernelandannulus_kernelare symmetric, so the usual path is fine; only asymmetric custom kernels are affected.Two changes:
convolution_2ddocstring spelling out the correlation convention, and that you can passkernel[::-1, ::-1]if you wantscipy.ndimage.convolvesemantics.Repro:
Also updates the reference-validation sweep state CSV. Verdict for convolution: CONVENTION-DIFF (interior matches scipy.ndimage.correlate exactly; correlation-vs-convolution naming was undocumented). Reference tool: scipy 1.16.1; gdal and astropy unavailable on this host; cupy parity confirmed on GPU.