Fix zppy testing - #1158
Merged
Merged
Conversation
…3.11 `plot_projection_comparison()` was the only one of the three comparison plotting functions that called `ax.set_title()` without an explicit `y`. Matplotlib only auto-positions a title when `y is None`, and that auto-positioning inspects the surrounding artists -- including the cartopy gridline labels, whose bounding boxes are non-finite while the figure is being drawn. The title therefore ended up at y = inf, which left the whole map axes with a non-finite tight bounding box (x0/x1 NaN, y1 inf). The subsequent `savefig(bbox_inches='tight')` in `shared/plot/save.py` unions only the finite axes -- the colorbars -- so the saved figure began at the first colorbar and the leading map panel was cropped out of the image entirely, with all three subplot titles missing. This is visible with matplotlib 3.11 (it does not reproduce on 3.10), and affects roughly 54% of the images in a zppy `comprehensive_v2` MPAS-Analysis run -- every ocean and sea-ice map that goes through this function. Passing an explicit `y` disables the auto-positioning, matching what `plot_polar_comparison()` (y=1.06) and `plot_global_comparison()` (y=1.02) already did. Reproducer, antarctic_extended sea ice concentration: before: 2620x1195 on matplotlib 3.11.1, 3575x1242 on 3.10.9 after: 3576x1242 on matplotlib 3.11.1, 3575x1242 on 3.10.9 Similar in spirit to E3SM-Project/polaris#635 and MPAS-Dev/compass#972. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`distutils` was removed from the standard library in Python 3.12, so `from distutils import dir_util` only kept working through the shim that setuptools installs. That made collecting anything under `mpas_analysis/test` fail outright in an environment without setuptools. `shutil.copytree(..., dirs_exist_ok=True)` is the direct replacement for `dir_util.copy_tree` onto an existing directory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dependencies are declared in four places -- `dev-spec.txt`, `pixi.toml`, `ci/recipe/recipe.yaml` and `pyproject.toml` -- and nothing kept them consistent, so they could drift silently. The new tests check that: * the three conda lists (dev-spec, pixi, rattler-build recipe) declare exactly the same runtime packages with the same version constraints * conda build pins (currently just the MPI flavor of ESMF) match between dev-spec and pixi * `pyproject.toml` agrees with the conda lists on every shared dependency, and the set it legitimately omits -- packages with no PyPI equivalent -- is exactly the documented `CONDA_ONLY` set, so adding a new conda-only dependency has to be a conscious choice * `requires-python` matches the `python` constraint in all four files * the dev and docs groups agree, allowing `pyproject.toml` to declare build-time requirements under `[build-system] requires` instead Differences in spelling are normalized: conda's `matplotlib-base` against PyPI's `matplotlib`, `_` against `-`, and pixi's `*` against an omitted constraint. The tests skip when run from an installed package, where the dependency files are not present. Verified that perturbing each of the four files is caught by the expected test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Running `pytest mpas_analysis/test` left four files behind in whatever directory pytest was started from, normally the root of the repo: inset_point.png inset_region.png inset_transect.png PET0.RegridWeightGen.Log The three PNGs came from `test_inset.py`, which passed bare relative filenames to `plt.savefig()`. The ESMF log is written by the `ESMF_RegridWeightGen` subprocess that pyremap launches from `test_climatology.py`; pyremap does not pass a `cwd`, so it lands in the current working directory and cannot be redirected from here. Add an autouse fixture that runs each test in its own temporary working directory. That covers the subprocess case, which is otherwise out of our control, and keeps future tests from reintroducing the problem. All tests find their inputs through absolute paths, so this is safe. Also have `test_inset.py` write to `tmp_path` explicitly rather than relying on the working directory, close its figures, and actually assert that the plot was written -- previously these three tests could not fail. The three near-identical tests become one parametrized test with the same three cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
TestingThe problem with matplotlib 3.11.1 shows up in: It looks right with the fix from this branch in: |
Collaborator
Author
19 tasks
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
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.


Fix panels dropped from projection comparison plots under matplotlib 3.11
Summary
Under matplotlib 3.11, roughly half the plots produced by an MPAS-Analysis run come out visibly broken: an entire map panel is cropped out of the figure and the subplot titles are missing. This is not a plotting-style regression that can be waved away as cosmetic, and it is not a change in the underlying data — the affected figures are genuinely unusable. This PR fixes it with a one-line change, and adds some unrelated test-hygiene cleanups that surfaced along the way.
The problem was originally reported against zppy's weekly integration testing, where 852 of 856 MPAS-Analysis images for
comprehensive_v2failed the image comparison: E3SM-Project/zppy#845Root cause
plot_projection_comparison()inmpas_analysis/shared/plot/climatology_map.pywas the only one of the three comparison plotting functions that calledax.set_title()without an explicity. That matters because matplotlib only auto-positions a title wheny is None, and that auto-positioning inspects the surrounding artists in order to place the title clear of them — including the cartopy gridline labels, whose bounding boxes are non-finite while the figure is being drawn.The title therefore ended up at
y = inf, which left the entire map axes with a non-finite tight bounding box. The subsequentsavefig(bbox_inches='tight')inmpas_analysis/shared/plot/save.pythen unions only the axes whose bounding boxes are finite — which is just the colorbars — so the saved figure began at the first colorbar and the leading map panel fell outside the image entirely.Instrumenting the three-panel sea ice concentration plot on matplotlib 3.11 shows this directly, where
ax0/ax2/ax4are the map panels andax1/ax3/ax5are their colorbars:The figure's bounding box starts at
x0=1505.7, which is exactly the left edge of the first colorbar rather than the left edge of the first map. After the fix every axes is finite and the figure bounding box starts atx0=550.0, the left edge of the first map panel.plot_polar_comparison()(y=1.06) andplot_global_comparison()(y=1.02) already passed an explicityand were never affected, which is why the bug is confined to the projection maps.The fix
Pass an explicit
yinplot_projection_comparison()so that matplotlib does not auto-position the title, matching what the other two functions already did. This is seven lines including the explanatory comment.This is the same class of problem addressed in E3SM-Project/polaris#635 and MPAS-Dev/compass#972, though the remedy differs: Polaris dropped
bbox_inches='tight'in favor of constrained layout, and Compass switched to requesting only the gridline labels it wanted. Requesting only the wanted labels was tried here first and did not fix it — the title still landed aty = inf— so the minimal change that actually works is the explicity. That also avoids raising the cartopy requirement to>=0.22, which list-valueddraw_labelswould have needed.Verification
Two pixi environments were built that differ only in the matplotlib version — python 3.14.7, numpy 2.5.2, cartopy 0.25.0, pillow 12.3.0 and freetype 2.14.3 are identical in both — and a full zppy
comprehensive_v2MPAS-Analysis run was done in each againstv2.LR.historical_0201on Chrysalis.Standalone reproducer, the
antarctic_extendedsea ice concentration comparison:Across a full 856-image run, comparing matplotlib 3.11.1 against 3.10.9 before the fix, 462 images (54%) differed by more than 10 pixels in at least one dimension — that is, broken layout rather than a text-metric shift — split 284 sea ice and 178 ocean. The remaining 372 differ by 10 pixels or fewer, which is the ordinary matplotlib 3.11 text-metric change and affects essentially every figure any version of this code produces.
After the fix, all 462 previously-broken images render correctly. Every one of the 178 ocean failures is resolved, and sea ice drops from 284 to 70. Those 70 remaining were inspected and are cosmetic: the height is identical in all 70 cases and only the width differs, by 11 to 16 pixels out of about 1620, and they are all
seaice_{area,volume}tend{thermo,transp}plots, whose long titles let text-metric drift accumulate past the 10-pixel cutoff used here. Comparing the fixed run against the broken run, exactly 462 images differ — precisely the set that was broken — and the remaining 394 are unchanged in size, so the fix touches nothing else.Also in this branch
These are independent cleanups that came out of getting the above tested, and can be split out if preferred.
distutilsremoved from the test fixtures.distutilsleft the standard library in Python 3.12, sofrom distutils import dir_utilonly kept working through the shim setuptools installs. In an environment without setuptools, collecting anything undermpas_analysis/testfailed outright.shutil.copytree(..., dirs_exist_ok=True)is the direct replacement.dev-spec.txt,pixi.toml,ci/recipe/recipe.yamlandpyproject.toml, and nothing kept them consistent. The new tests check that the three conda lists agree exactly, that conda build pins match, thatpyproject.tomlagrees on every shared dependency with the set it legitimately omits pinned to a documentedCONDA_ONLYlist, thatrequires-pythonmatches thepythonconstraint everywhere, and that the dev and docs groups agree. Perturbing each of the four files was verified to be caught by the expected test.pytest mpas_analysis/testleftinset_point.png,inset_region.png,inset_transect.pngandPET0.RegridWeightGen.Login the repository root. The PNGs came from bare relative filenames intest_inset.py; the ESMF log is written by theESMF_RegridWeightGensubprocess that pyremap launches, which gets nocwdand so cannot be redirected from here. An autouse fixture now runs each test in its own temporary working directory, which covers the subprocess case too.test_inset.pyadditionally writes totmp_pathexplicitly, closes its figures, and now actually asserts that the plot was written — previously those three tests could not fail.Note for downstream image comparisons
Any project comparing MPAS-Analysis output against stored reference images will need to regenerate its baselines when matplotlib 3.11 arrives, independently of this fix. matplotlib 3.11 changed text metrics slightly, so every figure saved with
bbox_inches='tight'comes out one or two pixels different in size and fails an exact-pixel comparison. That accounts for the 43% of images that were shifted but not broken.Checklist
Testingcomment in the PR documents testing used to verify the changes