Raise a clear error for non-2D custom kernels#2853
Merged
Conversation
custom_kernel unpacked kernel.shape into (rows, cols) without checking the kernel was 2D, so passing a 1D or 3D array to apply, focal_stats, or hotspots surfaced a raw 'not enough values to unpack' ValueError. Add an ndim check that reports the offending shape, and cover the three entry points plus custom_kernel directly.
brendancol
commented
Jun 2, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Raise a clear error for non-2D custom kernels
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
- convolution.py:333-335: the new ndim branch uses the same two-string-argument ValueError form as the existing odd-shape branch below it ("Received ...", "A custom kernel ..."). Worth knowing: passing two positional args to ValueError stores them as a 2-tuple in
args, so the printed message is the tuple rather than one sentence. That is a pre-existing pattern in this function, not something this PR introduced, and matching it is the right call. Noting it only for the record.
What looks good
- The check sits before
rows, cols = kernel.shape, so the cryptic unpacking error can no longer surface. - The message reports the bad shape via
kernel.shape, which reads correctly for both the 1D case (e.g. (3,)) and higher-dim cases (e.g. (3, 3, 3)). - Tests cover all three public entry points (apply, focal_stats, hotspots) with both 1D and 3D kernels, plus a direct unit test on custom_kernel. Each entry-point/kernel combo passes independently.
- The validation is backend-independent: it runs on the kernel argument before any ArrayTypeFunctionMapping dispatch, so numpy, cupy, dask+numpy, and dask+cupy all behave the same. No per-backend tests needed.
Checklist
- Algorithm matches reference/paper: n/a (validation-only change)
- All implemented backends produce consistent results: yes, validation runs pre-dispatch
- NaN handling is correct: n/a
- Edge cases covered by tests: yes (1D and 3D kernels)
- Dask chunk boundaries handled correctly: n/a
- No premature materialization or unnecessary copies: yes, no array ops added
- Benchmark exists or is not needed: not needed
- README feature matrix updated: not needed (no new function)
- Docstrings present and accurate: yes; the custom_kernel docstring ("Validates a custom kernel") still holds
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 #2842
custom_kernelinxrspatial/convolution.pyunpackedrows, cols = kernel.shapewithout first checking the kernel was 2D. A 1D or 3D kernel passed toapply,focal_stats, orhotspots(all of which route throughcustom_kernel) leaked a rawValueError: not enough values to unpack (expected 2, got 1).ndim != 2check incustom_kernelthat raises a clearValueErrorreporting the offending shape, before the shape unpacking.custom_kernelunit test.The check is backend-independent (it runs on the kernel argument before any dispatch), so it applies to numpy, cupy, dask+numpy, and dask+cupy alike.
Test plan: