Parallelize convolve_2d numpy kernel (#3615)#3616
Merged
Merged
Conversation
_convolve_2d_numpy iterated with numba.prange but was decorated @jit(nopython=True, nogil=True) with no parallel=True, so prange degraded to a serial range. On a 20-core host a 2000x2000 float64 raster with a 15x15 kernel dropped from ~356 ms to ~53 ms end-to-end once parallel=True was enabled, with identical results. Add parallel=True and serialize the kernel launch behind a module-level threading.Lock, since dask calls it per chunk under a threaded scheduler and numba's workqueue layer is not threadsafe across host threads (SIGABRT on macOS, same hazard as #3141 fixed in terrain.py). A single numpy call takes the lock uncontended and still runs across all cores. Add benchmarks/benchmarks/convolution.py (Convolve2d, CircleKernel) so the regression cannot silently return.
brendancol
commented
Jul 6, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Parallelize convolve_2d numpy kernel (#3615)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
-
benchmarks/benchmarks/convolution.py:20-21— the dask case oftime_convolve_2dnever runs the computation.convolve_2d(self.agg.data, self.kernel)on a dask-backed input returns a lazy array, so ASV times graph construction, not the kernel. Since this PR's whole point is dask throughput, the dask numbers won't move. Most dask benchmarks in this directory (watershed.py,twi.py,flood.py,interpolate.py,flow_length.py) callresult.data.compute()(or.compute()on the returned array) in thedaskbranch to force execution. Consider doing the same here.
Nits (optional improvements)
-
benchmarks/benchmarks/convolution.py:20-21— thecupycase doesn't synchronize the device, so the timing may end before the kernel finishes. This matches how the other cupy benchmarks in the repo are written, so it's consistent, just worth knowing the cupy number is a launch time rather than an execution time.
What looks good
- The
parallel=Truechange is guarded against the #3141 SIGABRT hazard exactly the way the terrain/reproject kernels were:_PARALLEL_KERNEL_LOCKserializes every kernel launch, and the dask path (the only concurrent caller, viamap_overlapunder the threaded scheduler) goes through_convolve_2d_numpy_locked. I checked every call site — there is no path that reaches_convolve_2d_numpywithout holding the lock. The comment on the lock spells out the reasoning for the next reader. - No data race inside the kernel: the outer
prangewrites disjointout[i, j], thenumaccumulator is loop-private, andout[:] = np.nanruns before the parallel region, so the boundary cells stay NaN. Numerical results are unchanged — only the outer loop is parallelized, inner accumulation order is untouched. - A benchmark now exists for
convolve_2dandcircle_kernel, which the module was missing.
Checklist
- Algorithm matches reference/paper (unchanged; only loop parallelism added)
- All implemented backends produce consistent results (numpy path unchanged numerically)
- NaN handling is correct (edge cells stay NaN)
- Dask chunk boundaries handled correctly (locked wrapper, depth unchanged)
- No premature materialization or unnecessary copies
- [~] Benchmark exists (added; dask case should force compute — see Suggestions)
- No macOS concurrent-kernel SIGABRT hazard (#3141) — serialized behind a lock
- Comments present and accurate
The dask case of time_convolve_2d returned a lazy array, so asv timed graph construction instead of the kernel this PR parallelizes. Force the compute for backends that produce a lazy array, matching the convention in the twi/interpolate/flood benchmarks. numpy and cupy results have no .compute and are unaffected. (#3615)
…e-convolution-2026-07-02
brendancol
commented
Jul 6, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review (follow-up): Parallelize convolve_2d numpy kernel (#3615)
Follow-up on commit ce8b097c, which addresses the one Suggestion from the first review.
Resolved
- The dask case of
time_convolve_2dnow forces the compute (benchmarks/benchmarks/convolution.py:22-23), so asv times the kernel rather than graph construction. The guard ishasattr(result, 'compute')on the returned array —convolve_2dreturns the bare backend array, so numpy (ndarray) and cupy (cupy.ndarray) skip it and only dask computes. I ran all three backend paths (this box has CUDA): numpy, cupy, and dask each construct and time without error, and the dask path materializes.
Merge with main
- Merged
origin/main(16 commits, including #3613's isort/unused-import cleanup onconvolution.py). The auto-merge kept both the style fixes and this branch'sthreadingimport +parallel=Truekernel. flake8 is clean onconvolution.pyand the benchmark, andtest_convolution.pypasses (6/6).
Still open
Nothing blocking. The cupy-doesn't-synchronize nit from the first review stands and remains consistent with the other cupy benchmarks in the repo, so I'd leave it.
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.
Fixes #3615.
Problem
_convolve_2d_numpyiterates its two outer loops withnumba.prangebut was decorated@jit(nopython=True, nogil=True)with noparallel=True. Numba only parallelizesprangewhenparallel=Trueis set, so as shipped the loop ran serial on a single core. This is the main CPU convolution path, used by the numpy backend and per chunk by the dask+numpy backend.Fix
parallel=Trueto the decorator.threading.Lock. Dask calls the kernel per chunk under its threaded scheduler, and numba's defaultworkqueuethreading layer is not threadsafe across host threads (SIGABRT on macOS). This is the same hazard and fix already applied toterrain.py(Streaming reproject thread pool aborts the process when numba parallel kernels run concurrently #3141). A single numpy call takes the lock uncontended and still runs across all cores; concurrent dask chunk calls run one at a time, each internally parallel.The cupy and dask+cupy paths use a separate CUDA kernel and are untouched.
Measurement
20-core host, 2000x2000 float64 raster, 15x15 kernel, end-to-end through
convolve_2d:Results are identical (verified
np.allclose(..., equal_nan=True)).Verification
xrspatial/tests/test_convolution.py: 6 passed.nan,nearest,reflect,wrap): match.Benchmark
Adds
benchmarks/benchmarks/convolution.pywithConvolve2d(numpy/cupy/dask across sizes and kernel sizes) andCircleKernel, so the serial regression cannot silently return.Performance context