fix: centre the Gaussian kernel and correct the CPU Scharr x-kernel - #58
Open
Hendrik-code wants to merge 1 commit into
Open
fix: centre the Gaussian kernel and correct the CPU Scharr x-kernel#58Hendrik-code wants to merge 1 commit into
Hendrik-code wants to merge 1 commit into
Conversation
Two convolution kernels that were not what their names say. Both are silent: the pipeline runs, nothing raises, the output is simply not the operation the config asked for. * get_gaussian_kernel1d sampled the Gaussian at arange(kernel_size) -- 0, 1, 2 -- putting the peak at index 0 rather than the centre tap. get_gaussian_kernel3d is the outer product of three of those, so its maximum sat at corner [0,0,0]. RandomGaussianBlurGPU and RandomUnsharpMaskGPU therefore blurred *and* translated the image by about a voxel. The segmentation mask is never convolved, so it did not move with it: the two came out misaligned. The kernel is now sampled on a centred linspace(-half, half, k). * The 2D CPU Scharr x-kernel had [-10, 0, -10] as its middle row. The whole kernel summed to -20 instead of 0, which makes it not a gradient operator -- it responds to constant regions. The sibling kernel_y on the next line has always been correct, which is what marks this as a typo rather than an intentional variant. The 3D tables are unaffected. unit_tests/test_kernel_correctness.py covers both, including the property that made the blur bug matter -- convolving a centred impulse must leave its centre of mass in place on every axis. 18 of its checks fail against the previous implementation. Models trained before this change saw the old behaviour and will not reproduce against it. No config key, parameter or default changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 20, 2026
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.
Stacked on #57 — review that one first; this PR's diff is only its own commit.
Two convolution kernels that were not what their names say. Both silent: the pipeline runs, nothing raises, the output is simply not the operation the config asked for.
The Gaussian was not centred
get_gaussian_kernel1dsampled atarange(kernel_size)— 0, 1, 2 — putting the peak at index 0 rather than the centre tap.get_gaussian_kernel3dis the outer product of three of those, so its maximum sat at corner[0, 0, 0].RandomGaussianBlurGPUandRandomUnsharpMaskGPUtherefore blurred and translated the image by about a voxel. The segmentation mask is never convolved, so it did not move with it — the two came out misaligned. The kernel is now sampled on a centredlinspace(-half, half, k).The 2D CPU Scharr x-kernel was not a gradient operator
The middle row should be
[-10, 0, 10]. Summing to −20 means it responds to constant regions.kernel_yon the very next line has always been correct, which is what marks this as a typo rather than an intentional variant. The 3D tables are unaffected.Testing
unit_tests/test_kernel_correctness.py, 13 tests / 23 subtests. 18 checks fail against the parent commit, verified by stashing the source change and re-running.The one that captures why the blur bug mattered:
test_the_old_uncentred_formula_really_was_off_centrepins the old formula explicitly so nobody reintroduces it as a simplification. The Scharr side asserts the defining properties — sums to zero, antisymmetric about its differencing axis, zero response on a constant image — plus a sanity check that it still detects the edge it is for, and a guard that the untouched Laplace branch of the same method still sums to zero.Full suite: 38 passed, 165 subtests.
ruffandmypyclean.Compatibility
Models trained before this change saw the old behaviour and will not reproduce against it. No config key, parameter or default changed.