Fix three critical OpenCL-convolution bugs#122
Open
paxcalpt wants to merge 1 commit into
Open
Conversation
Found during an audit of the website branch (these also affect PR #106, which carries the same opencl commit). 1. image_convolution.py: `convolve3D_new` is called but never defined anywhere, so the volume-convolution path raised NameError for every user (no GPU required). The line directly above already computes the result with the real `convolve3D`; the broken second assignment was a WIP leftover. Removed it. 2. image_convolution.py: the separable 1D kernels were applied to swapped axes — `ky` (the axis-1 line kernel[cz,:,cx]) was convolved along axis 0 and `kx` (the axis-0 line kernel[:,cy,cx]) along axis 1. Each extracted line must be convolved along the axis it varies on. Verified against a numpy/scipy reproduction: the swap gives ~7.4% error for a non-symmetric PSF, the corrected mapping gives 0. (GPU path itself not runtime-tested here — pyopencl is not installed in the audit env.) 3. pyproject.toml: package-data globbed only "*.yaml", so _convolution.cl was not shipped in the wheel and the GPU path crashed on a pip install. Added "*.cl". Verified the built wheel now contains the kernel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced May 29, 2026
There was a problem hiding this comment.
Pull request overview
This PR fixes correctness and packaging issues in the OpenCL-enabled 3D convolution path used by volume-convolution image generation.
Changes:
- Corrects separable OpenCL convolution axis application.
- Removes an undefined
convolve3D_newcall that caused runtime failures. - Includes
.clOpenCL kernel files in packaged distributions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/vlab4mic/utils/transform/image_convolution.py |
Fixes OpenCL separable convolution axis order and removes undefined fallback call. |
pyproject.toml |
Adds .cl files to setuptools package data so OpenCL kernels are included in wheels. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Found during an audit of the
websitebranch. All three verified; these also affect PR #106 (cl_conv), which carries the same OpenCL commit.1.
convolve3D_newis undefined — crashes for every user (no GPU needed)image_convolution.pyhad:convolve3D_newis defined nowhere in the repo, so the volume-convolution imaging path (generate_frames_volume_convolution) raisedNameErrorfor everyone, GPU or not. The first line already computes the result with the realconvolve3D; the second was a WIP leftover. Removed it. Verified: the module now imports andconvolve3D_newis no longer referenced.2. Separable 1D kernels applied to swapped axes — wrong GPU results
The separable decomposition extracts
kx = kernel[:,cy,cx](varies along axis 0),ky = kernel[cz,:,cx](axis 1),kz = kernel[cz,cy,:](axis 2), but appliedkyonaxis=0andkxonaxis=1. Each line must be convolved along the axis it varies on. Verified against a numpy/scipy reproduction: the swap produces ~7.4% error for a non-symmetric PSF; the corrected mapping gives 0. Invisible for axis-0/1-symmetric PSFs, wrong otherwise. (The GPU path itself isn't runtime-tested here —pyopenclisn't installed in the audit env — so the numerical check used the equivalent numpy logic.)3.
.clkernel not packaged — GPU path breaks onpip install[tool.setuptools.package-data]globbed only"*.yaml", so_convolution.clwasn't included in the wheel;_get_cl_code'sassert os.path.exists(...)would fail on an installed copy. Added"*.cl". Verified: a freshly built wheel now containsvlab4mic/utils/transform/_convolution.cl.Scope deliberately limited to the three critical correctness fixes. The remaining audit findings (per-call OpenCL context/recompile, dead fp64 check, single-device GPU-type check, debug print; and the mkdocs packaging items) are filed as issues.
🤖 Generated with Claude Code