fix(imq): correct FOCUS_SCORE overflow + vet 4 image-quality features vs analytic oracle - #394
Open
darkclad wants to merge 2 commits into
Open
fix(imq): correct FOCUS_SCORE overflow + vet 4 image-quality features vs analytic oracle#394darkclad wants to merge 2 commits into
darkclad wants to merge 2 commits into
Conversation
… vs analytic oracle
FOCUS_SCORE and LOCAL_FOCUS_SCORE were dominated by an unsigned-integer overflow: focus_score.cpp's
laplacian() multiplied PixIntens (unsigned) pixels by the negative Laplacian kernel weights, so e.g.
-4 wrapped to ~4.29e9 and FOCUS_SCORE reached 2.81e18. Cast the pixel to double before the multiply
-> FOCUS_SCORE 12.109, LOCAL_FOCUS_SCORE 2.681.
Vet 4 of the 6 imq features against an independent numpy/scipy reimplementation of their documented
algorithms (tests/vetting/oracles/gen_imq_analytic.py; reproduces every value exactly):
- FOCUS_SCORE = variance of the Laplacian magnitude (scipy convolution) = 12.109
- LOCAL_FOCUS_SCORE = same on the top-left h/2 x w/2 tile / 4 = 2.681
- MIN_SATURATION = fraction of pixels at the image minimum = 18/96 = 0.1875
- MAX_SATURATION = fraction of pixels at the image maximum = 16/96 = 0.16667
imq 0/6 -> 4/6.
The other two stay regression with documented reasons (oracle_coverage.csv notes):
- POWER_SPECTRUM_SLOPE: 0.0 on this fixture (rps() early-returns when min(h,w)/8 < 3), and its
radial binning uses the FFT value as the radius index instead of the frequency radius -- a real
bug; needs a rewrite + a >=24 px fixture.
- SHARPNESS: correct (all-double DOM sharpness port, no bug); needs a reference-DOM oracle.
gtest 717/717; pytest 72 passed (7 arrow fails are the tiff-only local build; the pytest imq tests
check the ImageQuality class runs, not feature values). Coverage 604 -> 608 / 758.
vjaganat90
suggested changes
Jul 29, 2026
| // Cast the pixel to double before multiplying: image is PixIntens (unsigned), | ||
| // so the negative kernel weights (e.g. -4) were converted to a huge unsigned | ||
| // value, wrapping the Laplacian (focus score reached ~1e18 from the overflow). | ||
| out[i* n_image + j] += (double)image[ii * n_image + jj] * kernel[ikFlip * n_kernel + jkFlip]; |
Member
There was a problem hiding this comment.
please use static_cast. If not now then open a PR where we port ALL C style casts to C++ safe casts.
| // FOCUS_SCORE = variance of the Laplacian magnitude, vetted vs scipy (gen_imq_analytic.py). | ||
| // Was 2.810e18: focus_score.cpp multiplied PixIntens (unsigned) by the negative kernel weights, | ||
| // wrapping the Laplacian; now cast to double. | ||
| double truth_value = 12.109375; |
Member
There was a problem hiding this comment.
is this the oracle value or regression value? In either case why did it change?
Member
There was a problem hiding this comment.
It's scipy oracle. This is resolved for me.
vjaganat90
approved these changes
Jul 29, 2026
vjaganat90
left a comment
vjaganat90
left a comment
Member
There was a problem hiding this comment.
LGTM. With the view that we will have a PR which fixes all the C-style casting redundant keywords in the codebase.
Address review on PR 394: replace C-style cast with static_cast<double>, and tag oracle references as 'Oracle: <tool>' in test comments and the coverage CSV so they read unambiguously. Also restores upstream's CRLF line endings for oracle_coverage.csv rows outside the IMQ family, which had been flattened to LF, inflating the diff.
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
2 participants
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.
FOCUS_SCORE and LOCAL_FOCUS_SCORE were dominated by an unsigned-integer overflow: focus_score.cpp's laplacian() multiplied PixIntens (unsigned) pixels by the negative Laplacian kernel weights, so e.g. -4 wrapped to ~4.29e9 and FOCUS_SCORE reached 2.81e18. Cast the pixel to double before the multiply -> FOCUS_SCORE 12.109, LOCAL_FOCUS_SCORE 2.681.
Vet 4 of the 6 imq features against an independent numpy/scipy reimplementation of their documented algorithms (tests/vetting/oracles/gen_imq_analytic.py; reproduces every value exactly):
imq 0/6 -> 4/6.
The other two stay regression with documented reasons (oracle_coverage.csv notes):
gtest 717/717; pytest 72 passed (7 arrow fails are the tiff-only local build; the pytest imq tests check the ImageQuality class runs, not feature values). Coverage 604 -> 608 / 758.