Rank image check failures by severity

[xylar]

Summary

Objectives:

• Rank image-check failures by how bad they are, so review starts with the worst and can stop once the rest are clearly cosmetic
• Stop reporting purely cosmetic differences (e.g. from a matplotlib upgrade) as failures
• Group failures by likely cause, so one example can stand in for hundreds
• Catch small but meaningful changes, such as a changed number in a plot's statistics

Issue resolution:

• Closes [Feature]: Account for pixel shifts in image checker tests #859
• Closes Account for harmless pixel shifts in image checks #860 (this replaces that approach)

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

The problem

The image check asked one question: do any pixels differ? For our plots the answer is nearly always yes. Upgrading matplotlib nudges every anti-aliased edge by a fraction of a pixel, so images that look identical differ in 2–20% of their pixels — far above the 0.02% threshold.

In the 2026-08-04 weekly test that meant 1274 of 1280 MPAS-Analysis images were reported as failures, in one flat alphabetical list, with nothing to say which ones mattered. Some were real bugs (a whole panel missing). Most were not. There is no threshold on that metric that separates them.

What this changes

Every image pair now gets a severity, and failures are sorted worst first.

Severity	Meaning	Action
STRUCTURAL	Figure changed size enough that a panel came or went	Investigate
MAJOR	A large part of the plot looks different	Investigate
MODERATE	A visible part of the plot looks different	Investigate
MINOR	Slightly different, or a small isolated change like a printed number	Skim
NEGLIGIBLE	Cosmetic only	Nothing — counted, not a failure

Two files are written next to the existing outputs:

• severity_report.txt — the ranked list. Starts with counts, then a grouping by likely cause, then every image needing review, worst first.
• image_scores.json — the same as raw numbers, so thresholds can be revisited without re-running the comparison.

On that same 2026-08-04 run: 516 images are now cosmetic and not reported, and the remaining 764 fall into 4 cause groups, so you check a handful of examples instead of scrolling 1274 filenames.

How it works, in one paragraph

Instead of comparing pixel to pixel, we ask of each pixel: does this colour appear anywhere within 4 pixels in the other image? If it does, the feature merely moved and we report nothing. If it does not, it genuinely changed or disappeared. That forgives the small reflow a matplotlib upgrade causes while still catching a deleted contour or a missing panel.

This is checked in both directions, which matters more than it sounds. If a contour line was deleted, the actual image is blank there — and blank pixels certainly do appear near the line in the expected image, so looking only one way sees nothing wrong. Only asking the reverse question notices the line is gone.

Why not #860

#860 assumed the whole figure slid by a fixed amount and tried to shift it back. Figures don't slide uniformly: the title, each panel and the colourbar all move by different amounts. To absorb that, #860 needed a tolerance so loose that real deletions passed silently. Letting each part of the image move a little on its own handles the same problem without that trade.

One case that needed separate handling

Severity is based on how much of the picture changed, which is not the same as how much it matters. A wrong digit in Mean -0.18 is about 13 pixels — roughly 240× smaller than the background noise level — so no area-based score can ever see it.

This is not hypothetical: our own e3sm_diags test fixture (tests/images/CRU-TREFHT-*) differs only by Mean -0.18 becoming Mean -0.17, and test_image_checker.py asserts it must be caught.

So there is a second, stricter check that looks for a few compact spots of strong difference. It finds the fixture, and returns exactly zero on 5357 of 5375 unchanged e3sm_diags images. Those images are marked MINOR with the cause "small isolated change (possible value change)" — worth knowing that this is the one group where a low rank does not mean a small problem.

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 considered likely and/or severe edge cases and have included them in testing.

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.

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. (New page: docs/source/dev_guide/tests/image_checking.rst.)

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.

[xylar]

Claude's response:

Testing

Unit tests

22 tests in tests/images/test_image_severity.py, using small synthetic images so they run anywhere without the expected-image trees on Chrysalis. They cover the cases that matter: a line that moved is forgiven, a line that was deleted is not, added/recoloured content is caught, and deletion stays detectable at every tolerance setting.

The existing tests/images/test_image_checker.py passes unchanged.

$ pytest tests/images/ -q
22 passed

pre-commit run --all-files passes.

Validation on real weekly-test output

Scored ~20,000 real image pairs across 3 weeks, both v2 and v3, and 4 tasks. "Old" is what the current checker reported.

Run	Task	Images	Old	Cosmetic	Needs review	Groups
08-04 v3	mpas_analysis	1280	1274	516	764	4
08-04 v2	mpas_analysis	856	852	376	480	4
08-28 v3	mpas_analysis	1280	1274	789	491	3
08-28 v3	e3sm_diags	5389	32	5366	23	2
07-31 v3	e3sm_diags	5389	6	5364	25	2
07-31 v2	e3sm_diags	3816	6	3801	15	2
07-31 v3	pcmdi_diags	647	646	607	40	2
07-31 v3	global_time_series	1404	1404	1404	0	0

Two things worth pulling out:

• e3sm_diags is the specificity check. 5389 images, almost all unchanged. It correctly leaves 5366 alone and flags 23 — no false alarms at MAJOR or STRUCTURAL. The 07-31 runs are from before the matplotlib change, and stay quiet too (the 20 and 10 flagged there are images that were never created).
• The 08-28 mpas_analysis run shows it tracking a real fix. The missing-panel bug was fixed between 08-04 and 08-28, and severity drops accordingly: 381 STRUCTURAL + 306 MAJOR on 08-04 becomes zero of either on 08-28.

One honest caveat on the last row: the 1404 global_time_series images that the old check flagged are now bit-identical to the expected images, so that list was stale. It is a useful specificity check (1404 identical images, zero flagged), not 1404 false positives removed.

What the output looks like

severity_report.txt, from a 40-image sample of the 08-04 run:

Image check for mpas_analysis
40 images compared
17 cosmetic (reported only, no review needed)
23 need review

Grouped by cause (check one example from each):
      12  MAJOR        title or label line added/removed
      10  STRUCTURAL   panel added/removed
       1  MODERATE     plotted content changed

Worst first:
  STRUCTURAL    1.0000  mpas_analysis/mvm/.../ocean/iceRunoffFlux_JFM_arctic_arctic_...png
  STRUCTURAL    1.0000  mpas_analysis/mvm/.../sea_ice/seaice_snowmeltNH_arctic_extended_...png
  ...
  MAJOR         0.1821  mpas_analysis/mvo/.../sea_ice/seaice_snowiceSH_antarctic_extended_...png
  ...
  MODERATE      0.0392  mpas_analysis/mvo/.../ocean/salinityTransect_OSNAP_East_...png

Those three groups were checked by eye and are all genuine:

• panel added/removed — expected has 3 panels (model / control / difference), actual has 2.
• title or label line added/removed — the v3.LR.historical_0051 subtitle is gone; the 47-pixel height loss is that line of text.
• plotted content changed — same size and layout, contour lines genuinely differ.

And image_scores.json alongside it:

{
 "name": "mpas_analysis/mvm/.../evaporationFlux_ANN_antarctic_antarctic_...png",
 "severity": "STRUCTURAL",
 "content_fraction": 1.0,
 "geometry_change": 0.262981,
 "localized_pixels": 0,
 "cause": "panel added/removed"
}

Where the thresholds came from

Hand-labelled 13 image pairs by looking at them, then swept the movement tolerance from 2 to 5 pixels. 4 pixels gave the best separation between images that were genuinely different and images that only looked different:

tolerance	separation
2 px	1.9×
3 px	3.4×
4 px	5.5×
5 px	6.9×

5 px separates slightly better but forgives more real movement, so 4 is the compromise. Two independent cross-checks: the split it produces (40% cosmetic / 55% real on the 08-04 run) closely matches the "43% cosmetic / 54% real" human assessment recorded in #859, which nothing here was tuned against.

Status of the remaining rough edge

The small-change check (§ "One case that needed separate handling" in the description) is the least settled part, so here is exactly where it stands.

It works, with a known false-positive rate that varies by task. On e3sm_diags it returns zero on 5357 of 5375 unchanged images (0.33% false positives). On pcmdi_diags the first version was much worse — 172 of 647 — because those plots have dense gridlines that all shift a pixel at once and produce spots everywhere. Requiring the spots to be few and in one place (a changed number is; shifted line art isn't) brought that to 36, and the last commit does exactly that.

What I could not solve: there is no way to catch this with the main severity score. A changed digit is ~13 pixels, about 240× below the noise floor, and I measured that floor three ways (resize, padding, integer alignment) — the best is ~0.0009, still far above 0.0000037. So it genuinely needs to be a separate check rather than a tuning change, and a separate check means a separate false positive rate. If those MINOR "possible value change" entries prove noisy in practice, the honest fix is OCR of the statistics boxes, which felt like too much machinery for this PR.

[xylar]

@forsyth2, I hope this makes sense as a replacement for #860 in your eyes. I think it will work better.

I don't know if it really requires incrementing the minor version number, since it is only a testing feature. Maybe Claude and I went overboard there.

I have not attempted to read the code carefully, instead focusing on the testing outcomes, which seem promising to me.

Claude is still working on improving the PDF produced as part of testing. Once that gets fixed, I've asked it to post an example.

[xylar]

Claude's response:

The diff grid PDF is still there, and now follows the ranking

It was never dropped — but it was the one output I had not actually exercised, because my end-to-end test wrote outside the web portal path and _make_image_diff_grid returns early there. Checking it properly turned up three things worth fixing.

Example (24-image sample of the 08-04 MPAS-Analysis run):

• image_diff_grid.pdf
• severity_report.txt
• image_scores.json

Of those 24 images, 9 are cosmetic and dropped; the PDF holds the remaining 15, worst first — 9 STRUCTURAL, then 6 MAJOR. Page 1 is a missing-panel case: actual has 2 panels where expected has 3.

What changed in the PDF

• Ordered worst-first, matching severity_report.txt, instead of filesystem walk order.
• Each row is labelled with its severity, so you can see how far down the ranking you are and stop when it stops mattering.
• Titles wrap. Paths are long enough that the title previously ran off both edges of the page and could not be read. This was pre-existing; adding the severity prefix would have made it worse.
• Columns are named actual / expected / diff, rather than relying on memory.
• pdf.savefig(1) → pdf.savefig(fig). The old call saved figure number 1 rather than the page just built. It happened to work only because matplotlib reuses the lowest free figure number once the previous page is closed — fragile, and it would have broken silently if anything else held a figure open.

Two unit tests now cover the ordering contract the PDF depends on, since the grid itself needs mache and a web-portal path and cannot easily be unit tested.

One caveat on the demo: the linked directory is a scratch copy under my public_html for review purposes, not a real test run. Happy to delete it once you have looked.

[xylar]

Claude's response:

Two follow-ups

1. Blank axes on the last page — fixed

You were right that this predates the PR. The last page is usually not full, and the leftover axes were still drawn, so the PDF ended with empty boxes carrying 0.0–1.0 tick labels — it read as though images had failed to load. They are now hidden.

The example PDF has been regenerated; page 8 now shows only its single real row.

While there, plt.subplots(..., squeeze=False) so axes stays two-dimensional. With rows_per_page=1 it would otherwise be a flat array of three axes and the row loop would iterate over individual axes rather than rows — latent, since the default is 2.

No unit test for this one: building a grid needs mache and a web-portal path, and checking the result needs a PDF renderer that is not a project dependency. Verified by rendering the final page before and after.

2. The missing subtitles were an MPAS-Analysis bug, and it is fixed

Not a mystery — MPAS-Analysis #1158, merged 2026-08-23.

plot_projection_comparison() called ax.set_title() without an explicit y. matplotlib only auto-positions a title when y is None, and that auto-positioning inspected cartopy gridline labels whose bounding boxes are non-finite during rendering. The title landed at y = inf, and savefig(bbox_inches="tight") then dropped the non-finite region — taking the leading map panel and the subtitle with it.

That single root cause explains both of the failure groups this PR's report separates out:

• panel added/removed — the cropped leading map panel
• title or label line added/removed — the dropped v3.LR.historical_0051 subtitle

The weekly runs bracket the fix, which makes a useful end-to-end check of the severity scoring. Taking the 302 images that scored title or label line added/removed on 08-04 and rescoring them on 08-28:

	08-04 (before #1158)	08-28 (after)
MAJOR	302	0
MODERATE	0	82
MINOR	0	164
NEGLIGIBLE	0	56

Figure heights go from 1399/1418 back to 1446, matching expected. Nothing remains MAJOR or STRUCTURAL.

So the calibration question I raised earlier — whether the missing subtitle was a real regression or an intended change — is settled: it was a real bug, it is fixed upstream, and the scorer ranked it as MAJOR while it was broken and stopped flagging it once it was fixed.

[xylar]

Claude's response:

Why the old PDF had 15 rows, not 24

Because 9 of those 24 scored NEGLIGIBLE. They get no diff images and no PDF row — that is the auto-pass doing its job. But it does mean the PDF alone cannot show you what is being dropped, so here is both halves.

1. A sample of every category

Re-sampled the 08-04 MPAS-Analysis run stratified by (severity, cause), 3 per group, rather than 24 at random. All four severity levels and all four causes now appear:

• image_diff_grid.pdf — 19 images, 10 pages
• severity_report.txt

       3  STRUCTURAL   panel added/removed
       3  MAJOR        title or label line added/removed
       3  MAJOR        layout shifted
       3  MODERATE     plotted content changed
       1  MODERATE     layout shifted
       3  MINOR        plotted content changed
       3  MINOR        layout shifted

Each row now also names its cause, not just its severity, so the PDF can be used the same way as the grouped report: check one example per group.

2. What is not getting flagged

Four cosmetic cases the old check reported as failures and this one drops. Left to right: expected, actual, difference amplified 8x so the noise is visible at all.

	old metric	new score
snowFlux_ANN_latlon	13.1% of pixels differ	0.00056
salinArgo_depth_-1500	11.6%	0.00031
temperatureWOA23_depth_-500	20.2%	0.00071
velocityMagnitude	30.1%	0.00128

Identical statistics (Min 1.221e-06 / Mean 0.003977 / Max 0.3084), yet 30% of pixels differ. The third panel shows why: every glyph, coastline and contour edge moved a fraction of a pixel, so the difference is spread over the entire figure rather than concentrated anywhere.

Same story across three panels — all six printed statistics match exactly, and the difference is confined to edges.

The other two: snowFlux, salinArgo.

There is also a cosmetic_sample PDF with 6 more, if it is easier to page through. That one is generated by a demo script, not by the checker — the checker deliberately does not write diff images for cosmetic results.

[forsyth2]

I hope this makes sense as a replacement

@xylar This is amazing, thank you! I had tried using CV to do some sort of categorization in a hackathon project (#727), but wasn't able to get it to a production-ready state. This categorization by severity is a great improvement.

I don't know if it really requires incrementing the minor version number, since it is only a testing feature.

I wouldn't worry too much about that. The PR template forces a choice (patch, minor, major), but realistically the version increment of the next zppy release is just a function of whatever the highest level PR seen is. That is, the choice here only matters if literally every other PR this development period is worth only a patch increment.

I have not attempted to read the code carefully, instead focusing on the testing outcomes, which seem promising to me.
Claude is still working on improving the PDF produced as part of testing. Once that gets fixed, I've asked it to post an example.

This example looks pretty nice -- the diffs are indeed marked structural, major, minor.

zppy is due for another weekly test, so I can try it out using the commits from this PR to really stress test it.

[forsyth2]

I think this PR could also close #849, as it removes much of the motivation for testing with frozen environments in the first place (i.e., "do we actually care about this diff?")

[forsyth2]

Following today's (9/4) test, I reran the image checker test using this PR's updates. Thanks to #861, doing so won't overwrite the image diffs seen on the main testing log.

Retest with this PR

cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/zppy
git status
# On branch test_zppy_20260904_run1
# Has uncommitted changes
git add -A
git commit -m "Testing" --no-verify
# Fetch the branch from https://github.com/E3SM-Project/zppy/pull/865
git fetch upstream better-image-diffs
git cherry-pick 0e1fbdd849e7a860f048a4c9dd7221415ae869f5
git cherry-pick 2a5831e79cc1907e90fe64b1c699f4d51018260c
git cherry-pick 0c9c61a4a52886f95f59a0cde2dc7effe8b6d854
git cherry-pick da50af1a7e152d32eaa58a48a824fcb2b27fccb6
git cherry-pick 91f35091e43c56fb71d7cc5adfb677886ed68406
git cherry-pick 168ed53f844bcdd9215eb724533502b9423011f0
git cherry-pick 330e5fcd9f406468988813093b4d0b1a612b8aee
git cherry-pick 0762c226baf7cec8ab1bf681bbebd100e819ab81
git log --oneline | head -n 10
# 7775d42f Name the likely cause on each diff grid row
# 8588ffb2 Hide unused axes on the last page of the diff grid
# e13ee3e0 Label and order the diff grid PDF by severity
# 2dd4d83c Ignore scattered spots when looking for changed values
# ce369bd5 Document how to review image check results
# 3bbacefd Rank image check failures by severity instead of listing them flat
# 31f3d9cd Detect small isolated changes such as a changed number
# 5c346f72 Add image severity scorer
# d0aa0826 Testing
# 77f4ab66 Append try# suffix to image_check_failures dir instead of overwriting (#861)

# The image checker test, which we'll run from a compute node:
salloc --nodes=1 --partition=debug --time=02:00:00 --account=e3sm
source /home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh
conda activate test-zppy-main-20260904_run1
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/zppy
python -m pip install .
pytest tests/integration/test_images.py
# Pasted `Captured stdout call` below

cat test_images_summary.md
# Pasted below

exit # Exit compute note

--------------------------------------------------------------- Captured stdout call ----------------------------------------------------------------
Preparing weekly cfg tests
Preparing legacy 3.1.0 cfg tests
Preparing legacy 3.0.0 cfg tests
Running 9 tests in parallel
Individual test logs will be written to test_<name>.log files
✗ Test bundles generated an exception: 
Copy the output of early_test_images_summary.md to a Pull Request comment
Copy the output of early_test_images_summary.md to a Pull Request comment
============================================================== short test summary info ==============================================================
FAILED tests/integration/test_images.py::test_images - AssertionError
========================================================== 1 failed in 3947.40s (1:05:47) ===========================================================

cat early_test_images_summary.md:

Test name	Total images	Correct images	Cosmetic only	Missing images	Needs review	Severity

I don't have time today to do a thorough review, but an early takeaway is that it lasted 4x longer (65 min instead of 15 min) than the 9/4 test itself and still didn't produce a test summary.

Next week I'll aim to debug the logs and also do a visual inspection of this PR.

[xylar]

Claude's response:

Thanks @forsyth2 — that run found two real bugs, both mine. Both are fixed, and I reran your exact test against your 9/4 outputs.

1. The crash

diff_dir is only written to by failures. When a config has nothing needing review, it was never created, and a bare assert fired:

else:
    # diff_dir won't exist if all the expected images are missing
    assert len(missing_images) == counter    # 0 == 1843 -> AssertionError('')

A bare assert has an empty message, which is why you got ✗ Test bundles generated an exception: with nothing after the colon, and an empty summary table. bundles is clean, so it hit this first and took the run down. It fires precisely when the checker is working. Fixed by creating the directory up front, which also gives the ranked report somewhere to live on a clean run.

2. The 4x slowdown

My fault, and my earlier comment was misleading about it: the "50s on 32 cores" figure came from my analysis scripts, which used a process pool. The shipped code is a serial loop. Measured per image it was 2.5–4.9x slower than the check it replaces, which matches your 65 minutes.

The fix came from profiling: 98.5% of pairs on your 9/4 run are byte-for-byte identical — only 150 pcmdi_diags images differ at all. Those can be settled from the file bytes without decoding either PNG.

	old check	this PR before	this PR now
e3sm_diags	238 ms/img	588 ms	44 ms
mpas_analysis	233 ms/img	1142 ms	30 ms

Rerun of your test, same data

I symlinked your 9/4 outputs into my www (your diff dirs excluded, so results land in mine) and ran all 9 configs:

8 minutes 2 seconds, versus 65:47 on your run and ~15 min for the 9/4 test itself. Full summary table produced. 50,655 images compared; 155 need review.

It still exits non-zero, which is correct — there are genuine differences:

| comprehensive_v3_pcmdi_diags | 647 | 497 | 497 | 0 | 150 | 45 structural, 105 major |
| comprehensive_v2_e3sm_diags  | 3870 | 3869 | 3869 | 0 | 1 | 1 minor, 3869 negligible |

The pcmdi_diags count is exactly the 150 your 9/4 run flagged, so we agree on what differs; this adds the ranking and grouping. I checked ts_ocean_AC.png by eye: real — those images gained y-axis latitude labels and a CORR 0.99 line.

On max_workers=6 — leave it alone

@xylar asked whether this should be raised to one worker per core. I tested it, and the answer is no: 12 workers takes 483.6s versus 482.3s for 6. Identical.

The pool has only 9 work items, one per config, and they are very unequal:

config	images	share
comprehensive_v3	9615	19.8%
legacy_3.1.0_comprehensive_v3	8540	17.6%
legacy_3.0.0_comprehensive_v3	7226	14.9%
the other six	5514 / 2234 each

comprehensive_v3 alone accounts for a fifth of the work, so it is the critical path and no amount of extra workers shortens it.

Worth knowing if this is ever revisited: the work is filesystem-latency bound, not CPU bound. A single process manages ~10 images/s, but 24 concurrent readers reach ~1360/s, because concurrency hides per-file latency rather than adding compute. So the lever is granularity, not worker count — splitting per (config, task) would give 33 items instead of 9 and cut the critical path to the largest single task. That is a change to the test harness rather than to this PR, so I have not touched it.

Cause labels reworded

Per @xylar's feedback, the labels were trying to name culprits the check cannot actually identify. ts_ocean_AC.png above is the example: labelled "panel added/removed" when no panel changed. They now describe what was measured — "figure size changed a lot", "figure height changed", "same size, content differs" — with the docs giving typical interpretations rather than pretending to be exhaustive.

One thing to know before reviewing

The expected-images baseline was regenerated on 9/4 at 11:55 — 6727 of 10007 v3 images and 4726 of 5514 v2. Presumably deliberate on your side, but it means the ~20,000-image validation table in my earlier comment was computed against a baseline that no longer exists. The conclusions about the method stand; those particular numbers no longer describe current behaviour.

Commits: 38be3d1e, a5894a9a, b6627be5, f737a548.