Account for harmless pixel shifts in image checks#860
Conversation
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances zppy’s integration image-checking utilities to tolerate small
pixel translations so semantically identical images don’t fail comparisons, while
keeping materially different images flagged. This aligns with zppy’s role as an
HPC workflow orchestrator by reducing noisy failures during automated validation
when underlying plotting dependencies change.
Changes:
- Refactors mismatched-pixel fraction logic into a helper and introduces
constants for thresholds. - Adds a shift-tolerant comparison path that attempts small translations before
declaring an image mismatch. - Adds a regression test ensuring small pixel shifts are ignored.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
tests/integration/image_checker.py |
Adds shift-tolerant image comparison helpers and threshold constants used by integration image checks. |
tests/images/test_image_checker.py |
Adds a pytest that creates a shifted image pair and verifies it no longer counts as a mismatch. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Important testing note: Simply re-running the 8/12 image checker test with this code would overwrite the existing image check failures directories linked there. |
|
Asking Claude to evaluate the performance impact of this diff, it gives: Net effect on your 30-60 min run Since the extra work only triggers for images that were already failing/borderline, the overall test suite time increase depends entirely on how many images currently sit above the 0.0002 mismatch threshold. If that's a handful of images, you'll see a small, likely unnoticeable increase (extra seconds per flagged image). If dozens of images are borderline, you could see a more noticeable bump, since each one now costs up to ~24x its diff-computation time. If you want to bound this cost, options include: caching/reusing intermediate crops, short-circuiting when image sizes are large (skip shift-matching above some resolution), or reducing |
| MAXIMUM_PIXEL_SHIFT = 2 | ||
| MAXIMUM_MISMATCH_FRACTION = 0.0002 |
There was a problem hiding this comment.
These seem to strenuous to be useful. If you take a look at the set of MPAS-Analysis diffs that were just generated, I would want a useful checker to detect those shifts. It should be possible to use a transform like an FFT to find out if the images are shifted by essentially any amount in x and y from each other in a single operation. The test should show a super strong peak a a fixed shift. I would think 0.02% of pixels changed would be overly rigorous but 2 pixels max shift is too few for sure.
|
@copilot Please address the following review comment. The current method of detecting pixel shifts appears to cause a up to a 24x slowdown according to Claude's analysis. A signal-processing approach (e.g., Fast Fourier Transform) may be both more efficient and more accurate. Review comment:
|
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
Replaced the exhaustive shift loop with a single FFT-based phase correlation call in
Correction: the commit hash is 060b783 (previous reply had a placeholder). |
|
Running this PR with the FFT commit (060b783) + #861 (avoid name collisions in image check failures directories), I don't actually see a change in results from the 8/28 run.
|
Co-authored-by: forsyth2 <30700190+forsyth2@users.noreply.github.com>
`_images_match_after_shift` previously only rescued a mismatched image if it was the exact same size as its expected counterpart and could be aligned by a whole-pixel translation. In practice, most real-world false-positive mismatches come from anti-aliasing/font-hinting differences after a matplotlib/FreeType/font version bump, which show up as a thin halo of differing pixels hugging every line, contour boundary, and piece of text - not a translation, and not always at matching image dimensions. This reworks the fallback check to: - Tolerate small (<= MAXIMUM_SIZE_DIFFERENCE px) size differences by resizing `actual` to `expected`'s dimensions before comparing, instead of failing immediately on any size mismatch. - Replace the single whole-image mismatch fraction with two zone-based fractions: `interior` (flat fills/backgrounds, must match almost exactly) and `edge zone` (near a line/contour/text in the expected image, given much more slack). Normalizing each independently by its own zone size - rather than the whole image - avoids penalizing images with more edge/text content than others. - Drop the early return when the estimated shift is (0, 0), so images that need no translation correction still get evaluated by the new zone-based check instead of being rejected outright. - Ignore per-pixel differences below DIFF_MAGNITUDE_THRESHOLD everywhere, since resizing alone introduces a few intensity levels of interpolation rounding even between genuinely identical images. The edge zone is always computed from `expected` only, so blurring introduced by resizing `actual` never distorts the zone boundaries used for classification. Known limitation: a small (~1-2% of image area) real content difference located inside an already edge-dense region of the plot may not be caught, since that's exactly where rendering noise is expected and tolerated. The mismatched-image diff grid remains useful for catching this narrow class of regression by eye. Diff generated by Claude (Anthropic); validated against real mismatched-image pairs from a comprehensive_v3 CI run and against synthetic shift/regression cases before being applied here.
|
Using the latest commits, the PR now generates a diff grid substantially different from the on seen on the 8/28 test run. 8/28 test run:
This PR:
The number of mismatched images has decreased by 1,228 (from 1,274 to 46). The number of correct images has increased by 1,228 (from 6 to 1234). Xylar noted here that all of the diffs from the 8/28 run were acceptable. That means this updated image checker correctly ignored 1,228 of the diffs it was bringing up before. It still however flagged 46. It should be noted the latest commit specifically notes:
Let's look at an example of one of these remaining diffs: expected & actual . Toggling between them, I notice that "actual"...
Overall this looks pretty promising and may remove alot of the hassle of checking pixel shifts going forward. |
|
@xylar Thanks for suggesting the FFT-based approach! Can you take a look at my review above and let me know what you think? If this looks good to you as well, I can incorporate these changes into future testing. |
|
@forsyth2, I was concerned if the periodicity of the FFT might be an issue so I ran the branch by Claude Opus. Here's what it said: I checked out the branch and ran the code against synthetic matplotlib figures. Good news on your question: the FFT periodicity is not causing problems. The bad news is that the check running after it is. FFT is fineSign convention is correct and matches what One gap: there's no peak-quality gate, which is what you originally asked for ("should show a super strong peak"). Two unrelated plots gave The real issue: the edge zone swallows everything
Deleting every contour line passes silently. So some of the 1228 newly-"correct" images may be passing for this reason rather than because a shift was correctly recognized. What forces the slack is the resize path, not the shifts. For same-size images, genuine shift and anti-aliasing noise both score exactly 0.0 on the edge fraction, so it could be ~0.01 instead of 0.5. The 0.5 is only needed to survive Two smaller things
Suggested next stepThe FFT part looks good to merge. Before trusting the zone check, could you have it print the |
|
So I think the concept of the FFT is okay but maybe there's a better implementation. Another transform like a wavelet or a cosine transform might actually be more appropriate for images that aren't periodic. |
- Move size-difference handling before the diff is computed. ImageChops.difference() doesn't raise on mismatched sizes -- it silently returns just the overlapping intersection, so a real change outside that intersection could go uncounted. actual is now resized (or the pair is flagged as mismatched) before any diff is taken, and both the compare and shift-matching code path work only with already-matched sizes. - Split the edge-zone mismatch tolerance in two, based on whether actual had to be resized: MAXIMUM_EDGE_ZONE_MISMATCH_FRACTION_SAME_SIZE (0.01) for same-size comparisons, MAXIMUM_EDGE_ZONE_MISMATCH_FRACTION_RESIZED (0.5, unchanged) when a resize occurred. The old single 50% tolerance applied everywhere let real content changes hide in edge-dense regions, since up to ~66% of a contour-dense plot can fall inside the edge zone; the wide margin is only actually needed to absorb resize-interpolation noise. - Add a peak-quality gate to the FFT-based shift estimate. _estimate_shift now also returns the correlation value at its peak (bounded in [0, 1]); below MINIMUM_SHIFT_PEAK_QUALITY (0.5, a starting point pending real calibration data) the estimated shift is discarded and treated as "no shift found" rather than accepted at face value. - Add an opt-in ZPPY_IMAGE_CHECK_DIAGNOSTICS env var that prints, per image, whether it was resized, the estimated shift and peak quality, and both mismatch fractions against whichever threshold applied -- for validating the thresholds above against a full image corpus. - Add test_compare_flags_real_change_in_edge_dense_region, a regression test for a real content change landing entirely inside an edge-dense region (~98% edge coverage), which would have silently passed under the old single-threshold behavior and correctly fails now. Generated by Claude.
I did a little more iteration with Claude, which led to the latest commit, f2bddc5. It decided FFT was still appropriate, but made a few updates.
Let's compare that with previous runs:
So, these changes are actually showing 1234-1229=5 fewer images marked as correct. However, we expected the latest commit to increase the number of correct images. It also turns out that the diff is actually 17 newly-mismatched offset by 12 newly-correct (17-12=5). Diff detailsdiff /lcrc/group/e3sm/public_html/diagnostic_output/ac.forsyth2/zppy_weekly_comprehensive_v3_www/zppy_main_branch_test_20260828_run1/v3.LR.historical_0051/image_check_failures_comprehensive_v3_try5/mpas_analysis/mismatched_images.txt /lcrc/group/e3sm/public_html/diagnostic_output/ac.forsyth2/zppy_weekly_comprehensive_v3_www/zppy_main_branch_test_20260828_run1/v3.LR.historical_0051/image_check_failures_comprehensive_v3_try6/mpas_analysis/mismatched_images.txt > diff_try5_try6.txt
cat diff_try5_try6.txtgives: @xylar Do you have any further comments? Should I keep or drop the latest commit? I don't have a lot of background in FFTs and other transforms, so I'm relying on Claude heavily here. (That said, this is an interesting and useful method, so I'd like to learn more). It would be nice to get this PR to a merge-able state since it would be extremely useful to systematically ignore pixel-shift diffs, but I certainly don't want to end up accidentally removing meaningful diffs in the process. |
|
Addendum from Claude, based on comparing a near-identical expected & actual pair that showed up as mismatched. traced the root cause of the 17 new mismatches — it's not a shift issue. This means the edge-zone tightening in the latest commit (0.5 → 0.01 for same-size images) was based on a false premise: I'd assumed genuine same-size noise stays near 0%, but this benign, harmless drift measures 14-31%. A 1% ceiling can't survive that. Recommendation: keep the resize-order fix and the peak-quality gate (both hold up under scrutiny), but revert the threshold split back to a single ~0.5 value for now. That knowingly re-opens the "loose tolerance hides a deleted contour" gap you originally flagged — I don't have a real fix for that yet, just don't want to ship a number I now know is wrong. |
Summary
Objectives:
Select one: This pull request is...
Small Change
Big Change
1. Does this do what we want it to do?
Required:
If applicable:
2. Are the implementation details accurate & efficient?
Required:
If applicable:
zppy/conda, not just animportstatement.3. Is this well documented?
Required:
4. Is this code clean?
Required: