Skip to content

Account for harmless pixel shifts in image checks - #860

Draft
forsyth2 with Copilot wants to merge 6 commits into
mainfrom
copilot/add-account-for-pixel-shifts
Draft

Account for harmless pixel shifts in image checks#860
forsyth2 with Copilot wants to merge 6 commits into
mainfrom
copilot/add-account-for-pixel-shifts

Conversation

Copilot AI commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Objectives:

  • Ignore semantically identical images with small pixel shifts.
  • Preserve failures for material image differences.

Select one: This pull request is...

  • a bug fix: increment the patch version
  • a small improvement: increment the minor version
  • a new feature: increment the minor version
  • an incompatible (non-backwards compatible) API change: increment the major version

Small Change

  • To merge, I will use "Squash and merge". That is, this change should be a single commit.
  • Logic: I have visually inspected the entire pull request myself.
  • Pre-commit checks: All the pre-commits checks have passed.

Big Change

  • To merge, I will use "Create a merge commit". That is, this change is large enough to require multiple units of work (i.e., it should be multiple commits).

1. Does this do what we want it to do?

Required:

  • Product Management: I have confirmed with the stakeholders that the objectives above are correct and complete.
  • Testing: I have added or modified at least one "min-case" configuration file to test this change. Every objective above is represented in at least one cfg.
  • Testing: I have considered likely and/or severe edge cases and have included them in testing.

If applicable:

  • Testing: this pull request introduces an important feature or bug fix that we must test often. I have updated the weekly-test configuration files, not just a "min-case" one.
  • Testing: this pull request adds at least one new possible parameter to the cfg. I have tested using this parameter with and without any other parameter that may interact with it.

2. Are the implementation details accurate & efficient?

Required:

  • Logic: I have visually inspected the entire pull request myself.
  • Logic: I have left GitHub comments highlighting important pieces of code logic. I have had these code blocks reviewed by at least one other team member.

If applicable:

  • Dependencies: This pull request introduces a new dependency. I have discussed this requirement with at least one other team member. The dependency is noted in zppy/conda, not just an import statement.

3. Is this well documented?

Required:

  • Documentation: by looking at the docs, a new user could easily understand the functionality introduced by this pull request.

4. Is this code clean?

Required:

  • Readability: The code is as simple as possible and well-commented, such that a new team member could understand what's happening.
  • Pre-commit checks: All the pre-commits checks have passed.

Copilot AI linked an issue Aug 28, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Add account for pixel shifts in image checker tests Account for harmless pixel shifts in image checks Aug 28, 2026
Copilot AI requested a review from forsyth2 August 28, 2026 21:51
Copilot AI and others added 4 commits August 28, 2026 21:58
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>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread tests/images/test_image_checker.py Outdated
Comment thread tests/integration/image_checker.py
Comment thread tests/integration/image_checker.py Outdated
Comment thread tests/integration/image_checker.py
@forsyth2

Copy link
Copy Markdown
Collaborator

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.

@forsyth2

Copy link
Copy Markdown
Collaborator

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 if ±2 is more generous than needed.

Comment on lines +12 to +13
MAXIMUM_PIXEL_SHIFT = 2
MAXIMUM_MISMATCH_FRACTION = 0.0002

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Account for pixel shifts in image checker tests

4 participants