Repository navigation
Round to nearest in every scale_brightness(), and make the ImageScaleBrightness test a real test - #1669
Merged
Conversation
Collaborator
|
Gin, can you split this up into 3 smaller PRs? One for each kernel that you're changing:
|
Collaborator
Make all of them round to nearest, including the default. So instead of changing the SIMD ones, change the default to I don't care if the exact 0.5 rounding behavior differs slightly between Default and SIMD. That's considered a ULP error which is generally accepted across all our kernels. |
The x86 SIMD versions round each channel to the nearest integer, but Default and NEON truncated, so results on Apple Silicon were up to 1 lower per channel. Now Default and NEON round too. At an exact .5 tie, Default rounds up and the SIMD code rounds to even. That 1-unit difference is accepted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The old test was debug code: it was not registered, and it only timed the implementation that the CPU dispatch picks. Now each implementation that the build has and the CPU supports gets a test on 4 CommandLineTests images. It checks that each channel is rounded to nearest, and accepts either value at an exact .5 tie. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Gin890
force-pushed
the
test-kernels-real-images
branch
from
October 23, 2026 04:12
18c1417 to
363c543
Compare
Gin890
changed the title
Collaborator
Author
|
Split into three commits as suggested and changed this PR to round to nearest for image kernel tests @Mysticial |
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.
Make every
scale_brightness()implementation round each channel to the nearest integer, and make the ImageScaleBrightness kernel test a real test.As Mysticial asked, this PR now has only the
scale_brightness()part. The other two parts are in #1681 (PackedBinaryMatrix::submatrix()) and #1682 (filter_rgb32_range()).The x86 SIMD versions already round to nearest, but Default and NEON truncated. So on Apple Silicon,
ExactImageMatcherandImageMatchDetectorgot channels up to 1 lower than on x86. Now Default and NEON round too. At an exact .5 tie, Default rounds up and the SIMD code rounds to even. The test accepts both.Before, the test was debug code. It was not registered, so CI did not run it. It only timed the implementation that the dispatch picks, and did not check the result. Now it checks each implementation that the CPU supports, on real images.
The images are from the CommandLineTests repo. Their widths cover every remainder mod 4, so the test also checks the leftover pixels after the SIMD loops. #1554 also changes this test file, so it will need a small merge fix.
0 test files added, 1 test file changed:
Test results: added when the CI of this PR finishes.
🤖 Generated with Claude Code