test(convolution): backend, boundary, and metadata coverage#3621
Open
brendancol wants to merge 2 commits into
Open
test(convolution): backend, boundary, and metadata coverage#3621brendancol wants to merge 2 commits into
brendancol wants to merge 2 commits into
Conversation
Deep-sweep test-coverage on a CUDA host. Branch coverage was 52% with the test file exercising only numpy error paths and the default boundary mode. convolve_2d/convolution_2d dispatch to numpy, cupy, dask+numpy, and dask+cupy, but only the numpy path had tests. Add: - Backend parity: convolution_2d numpy vs dask+numpy, cupy, dask+cupy (Cat 1). Ran and passed on the GPU host. - Boundary modes: nan/nearest/reflect/wrap on numpy+dask and a cupy/dask+cupy-vs-numpy loop; invalid-boundary ValueError (Cat 4). - Metadata: convolution_2d preserves coords/dims/attrs/name (Cat 5). - NaN-interior propagation (Cat 2); degenerate 1x1/1xN/Nx1 all-NaN (Cat 3). - Previously untested public funcs: calc_cellsize, annulus_kernel, custom_kernel valid/non-ndarray/even-dims, circle_kernel bad radius/unit. Full file: 26 passed including 3 GPU tests. No source change; no bug surfaced. flake8/isort clean.
brendancol
commented
Jul 6, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: test(convolution): backend, boundary, and metadata coverage
Test-only PR. xrspatial/tests/test_convolution.py gains 189 lines closing the
backend-parity, boundary-mode, and metadata gaps in convolution.py, plus first
tests for calc_cellsize, annulus_kernel, custom_kernel, and circle_kernel.
convolution.py is untouched. I read the source and the general_checks.py
helpers, ran the full file, and confirmed the assertions line up.
Verification (ran locally on the CUDA host)
pytest xrspatial/tests/test_convolution.py: 26 passed.- The three GPU tests (
test_convolution_2d_numpy_equals_cupy,
test_convolution_2d_numpy_equals_dask_cupy,
test_convolution_2d_boundary_modes_gpu_match_numpy) ran for real, not skipped. - flake8 and isort clean.
- Import resolved into the PR worktree, not the main editable install.
Blockers
None.
Suggestions
None blocking.
Nits
test_convolution_2d_boundary_modes_gpu_match_numpyre-implements the
boundary-mode loop by hand (.data.get()/.data.compute().get()) instead of
going throughassert_boundary_mode_correctness. That is because the helper
only compares numpy against a dask arg, so there is no shared path for the
cupy/dask+cupy comparison. Fine as written; worth a helper only if a later PR
adds more GPU boundary tests.- The degenerate-shape and NaN-interior tests pin current behavior (all-NaN
output, NaN propagation through the plus-kernel window) rather than a reference
value. That matches the algorithm and the PR's intent, so no change needed.
What looks good
- Backend parity reuses the
assert_numpy_equals_*helpers, so the numpy path is
the reference for all three other backends. - Boundary coverage hits all four public modes (
nan/nearest/reflect/wrap)
on numpy, dask, cupy, and dask+cupy, plus the invalid-modeValueError. - The NaN-interior test picks cells whose 3x3 circle window overlaps the seeded
NaN and one that does not, so it checks propagation and non-propagation. - Kernel-generator error paths (non-ndarray, even dims, bad radius/unit) are
covered.
Checklist
- Algorithm matches reference (test-only; assertions match source behavior)
- All implemented backends produce consistent results (parity tests, GPU ran)
- NaN handling is correct (interior propagation + boundary='nan' edges)
- Edge cases covered (1x1/1xN/Nx1 degenerate, invalid boundary, bad kernel)
- Dask chunk boundaries handled (boundary-mode helper compares dask vs numpy)
- No premature materialization or unnecessary copies (test-only)
- Benchmark exists or is not needed (test-only, no perf change)
- README feature matrix updated (n/a, no new public function)
- Docstrings present and accurate (n/a for tests; source docstrings unchanged)
…age-convolution-2026-07-02
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.
What
Adds tests for
xrspatial/convolution.py. Before this, the test file had 5 teststhat only touched numpy error paths and the default boundary mode, so branch
coverage sat at 52%.
convolve_2dandconvolution_2ddispatch across numpy, cupy, dask+numpy, anddask+cupy, but only the numpy path was exercised. The boundary parameter has four
public modes and only the default was tested.
convolution_2dre-attachescoords/dims/attrs/name and nothing checked that it does.
New tests
convolution_2dnumpy vs dask+numpy, cupy, and dask+cupy.vs numpy comparison over all four modes. Plus the invalid-boundary ValueError.
convolution_2dpreserves coords, dims, attrs, and name; custom name.calc_cellsize(res attr and km->m unitconversion),
annulus_kernel,custom_kernelvalid/non-ndarray/even-dims, andcircle_kernelbad radius/unit.Verification
Ran on a CUDA host, so the cupy and dask+cupy tests executed for real, not just
static review. Full file: 26 passed, including the 3 GPU tests. flake8 and isort
clean.
No source change. No bug surfaced by the new tests. This is test-only.
Branch coverage went from 52% to 72% as measured on CPU (
NUMBA_DISABLE_JIT=1).The cupy kernel lines can't be seen by CPU coverage instrumentation, but they run
under the 3 passing GPU tests.
Found by the test-coverage deep sweep.