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.
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: