Make re-running the plots cheap, and convert units exactly once (#425)

[brianpm-ucar]

Closes #425, and covers a good part of #409.

Two things turned out to be tangled together. Re-running the ADF with redo_plot: false was doing far more than deciding there was nothing to do, and the reason a plotting script had to open its data before it could decide was often the same reason it then handled units wrong.

Measured

A two-case, ten-variable run whose plots all already existed, redo_plot: false:

	Before	After
plotting stage	13.6 s	~1.2 s
files rewritten	5	0

The five rewritten files

zonal_mean built the log-pressure file name two different ways. The pre-flight pass looked for {var}_{season}_Zonal_logp_Mean.png; the plotting pass wrote {var}_logp_{season}_Zonal_Mean.png. The name it looked for never existed, so every log-pressure plot was redrawn on every run whatever redo_plot said — and that redraw was the 3.6 s in matplotlib's _update_ticks and 3.1 s in mathtext.parse that dominated the profile. One 3-D variable here; a run with many would redraw five plots per variable, every time.

zonal_mean also computed both seasonal means before finding out whether either plot was wanted.

Deciding without opening the data

A script could not tell what it would produce without knowing whether the variable has a lev dimension, which is a property of the data — so it opened the data. But a 2-D variable and a 3-D one are named differently, so trying both spellings answers both questions at once: whether anything is missing, and which kind of variable it was. That is plotting_utils.first_complete_plot_set, used by polar_map and global_latlon_vect_map.

Scripts changed:

• meridional_mean — rewritten to follow zonal_mean, which is what Clean up Meridional plots to mimic code of Zonal plots #425 asks for: a pre-flight pass, and data read through AdfData rather than a hand-rolled glob of cam_regrid_loc. None of its names depend on lev, so a variable or case whose plots exist is settled from the names alone.
• polar_map — worked out that everything existed, but opened a regridded file per case first, only to look for lev.
• global_latlon_vect_map — opened four regridded files for a vector pair before checking.
• global_mean_timeseries — opened the reference time series and every case's, and took global means of them, before conditional_save decided not to write.
• aod_latlon — opened both observation files and every case's climatology up front, then checked each panel.

Already correct and left alone: qbo, tape_recorder, adf_histogram, cam_taylor_diagram.

Units: one decision instead of seven

Migrating meridional_mean to AdfData exposed a second bug, and pulling on it found the general case. Whether a conversion had already been applied was being decided seven different ways:

Loader	Policy
load_da, both time series loaders	applied unconditionally
load_climo_ds	unconditional, then stamped transformed
load_reference_climo_ds	stamp or units == new_unit
load_reference_climo_da	no check at all — same files as the line above
the two regrid loaders	stamp only

That units == new_unit was a literal string comparison, so it almost never fired: CAM writes W/m2 where the defaults say Wm$^{-2}$. There is now one decision — AdfData.already_converted — used by all of them: the stamp if it is there, otherwise whether the file's units already say what the conversion is converting to.

lib/adf_units.py carries that comparison. It reduces a unit to a canonical form first, so the spellings agree:

W/m2 · W m-2 · Wm^-2 · W m**-2 · W m⁻² · W m$^{-2}$ · Wm$^{-2}$   → one unit
kg/kg · kg kg-1 · fraction · Fraction                            → dimensionless
m/s vs s/m · Pa vs hPa · ppbv vs ppmv · W/m2 vs W/m3              → correctly unequal

It folds a solidus into negative exponents, splits a run-together factor into known symbols, reads superscripts, and strips LaTeX. hPa is deliberately not split into hour-pascal. This is not hypothetical — adf_variable_defaults.yaml contains both Wm$^{-2}$ and W m$^{-2}$ today. The module imports nothing but re, so CI runs its tests.

Three scripts were scaling twice

The regridding stage applies the variable-defaults conversion when it writes and stamps transformed; these applied scale_factor and add_offset again on top of it:

• meridional_mean — PRECT was drawn 86400000 times too large and PS a hundred times too small
• global_latlon_vect_map — on regridded climatologies, and it also relabelled units from the defaults whether or not it had converted, claiming units the data was not in
• tem

All three now go through AdfData.apply_conversion, which makes the same decision the loaders do.

What changes in the output

Of the thirty meridional plots, the fifteen for variables with no unit conversion (TS, ICEFRAC, Q) are byte-identical. PRECT and PS change because they were wrong.

RESTOM changes for a third reason, and it is not a bug fix. load_da applies new_unit only alongside a conversion, so RESTOM's label goes from the defaults' W m$^{-2}$ to the file's own W/m2 — the same units, rendered differently. That is what zonal_mean has always shown, so this makes meridional agree with it rather than introducing something new. If the preference is that new_unit should relabel even when nothing is converted, that is a one-line change in load_da — but it would relabel plots across every script, so it belongs in its own PR.

Testing

New CI-runnable tests: test_adf_units.py (8 tests, 25 subtests) covering the spelling families, the pairs that actually occur in this repo, units that must stay different, dimensionless forms, missing units matching nothing, and hPa not being split. test_regrid_scaling.py gains two cases for the units evidence and now exercises the real shared decision rather than a literal stamp check.

End to end on Casper, model vs model, two dissimilar cases:

• the AMWG tables are byte-identical to what main produces
• a plotting re-run rewrites nothing, where it used to rewrite five files
• the vector and global-mean paths verified by adding U and V: ten vector plots and five global-mean plots on the first pass, nothing on the second
• aod_latlon's pre-flight tested directly with synthetic panels (no AOD data on this machine): all present → registers exactly 8; one missing → registers none; redo_plot: true → does not fire
• I checked the dangerous direction of the units heuristic — a conversion that does not change the units would be wrongly skipped — and found 0 variables at risk among those with real data. TAUX/TAUY, scaled by -1, are safe because they have no new_unit and so rely on the stamp alone.

pytest lib/test/unit_tests: 167 passed in a full environment; 76 passed, 68 skipped in a virtualenv holding only PyYAML and pytest, the way CI builds it. darker --check --revision main clean.

Not in this PR

regional_map_multicase reads redo_plot and never uses it, so it always regenerates; cloud_regime_analysis has no redo handling at all. tem, MOPITT and enso_comparison_plots still open their data before checking. Left deliberately for a follow-up on #409.

🤖 Generated with Claude Code

[brianpm-ucar]

Review round: a crash, a wrong unit comparison, and six smaller things (99d3ac8d)

A fresh Claude-assisted adversarial review against the repo's AGENTS.md found two blocking defects and several smaller ones. All reproduce, all are fixed. Posting the findings because one of them would have broken every TEM run.

1. tem.py raised NameError on the first plot it drew

The entry point is def tem(adf), and every other line uses adf. My two new lines used adfobj:

mdata = adfobj.data.apply_conversion(mdata, var)   # NameError

Any config with tem in plotting_scripts — the shipped config_cam_baseline_example.yaml configures TEM — would have hit this as soon as one TEM plot needed drawing, losing the rest of the run. A re-run whose plots all already existed hits the continue first, which is exactly how my own end-to-end testing missed it. I have no TEM data on this machine, which is the underlying gap.

2. normalize_units reported two different units as equal

The run-together splitter matched known symbols greedily after lower-casing, so:

'Nm-2' -> 'nm^-2'      newton per metre squared
'nm-2' -> 'nm^-2'      per square nanometre        <- reported equal

That is precisely the false match the module exists to prevent, and the same greediness shredded unit names that happen to spell other symbols — Sv became siemens-volt, cal became c-a-l, dam became d-a-m.

The splitter is gone. In its place is an explicit table of the run-together forms that actually occur (Wm, Nm, ms, kgm, …), applied while the case is still intact, so Nm and nm can never collide. A unit name it does not know is left whole. N/m2 and Nm-2 still agree; Nm-2 and nm-2 no longer do. Multi-factor units written with several solidi — kg/m2/s, W/m2/K, 1/s, m3/m3 — now normalise correctly too, which the greedy version got wrong.

Separately, a ratio whose factors cancel now keeps what cancelled. kg/kg and kg kg-1 still agree, but kg/kg and mol/mol no longer do: a mass mixing ratio and a volume mixing ratio are both dimensionless and are not the same number. No shipped default has a dimensionless new_unit, so this was latent — but it was the most dangerous entry in the table for a chemistry variable someone adds later.

Also fixed

• load_reference_regrid_da did not pass field=, unlike its climo sibling, so for observations the new guard in load_da looked the obs variable name up in the variable defaults, found nothing, and could never fire. Nothing was double-scaled (_regrid_converters still checked correctly), but the guard was dead code on that path.
• load_reference_climo_ds stamped transformed but never set the new units. Confirmed on disk: ceresmip_amip02_PRECT_baseline.nc says units='m/s' over a mean of 2.397 — i.e. mm/day. The units string was a lie. Its test-case sibling load_climo_ds sets it; now both do.
• first_complete_plot_set was being given the layouts least-specific-first, against what its docstring says, so a 3-D variable with stale 2-D-named plots could be declared complete and its levelled plots never drawn. Both callers now pass [levelled, flat].
• adf_units.py is now in the set of files CI lints. It imports nothing but re and scores 10.00/10, so gating it is free, and it is the highest-risk new logic in the PR.
• plotting_utils's module docstring started with a bare .; meridional_mean's Notes did not list compare_obs, which the rewrite now uses; a leftover implicit string concatenation in aod_latlon.
• plot_meridional_mean_and_save assigned the reference's time mean to adata, throwing away the latitude weighting just applied to the test case. Latent, because seasonal_mean(is_climo=True) removes time before the call, so the branch never fires today. Fixed here rather than deferred because Clean up Meridional plots to mimic code of Zonal plots #425 is about the meridional plots.

One finding I checked and did not act on

The review flagged regrid_and_vert_interp._find_surface_pressure for loading the test PS through load_climo_ds (hPa) and the reference PS through load_dataset (Pa). The asymmetry is real but deliberate: both call sites pass the result through _pressure_in_pa, which normalises by the units attribute and falls back to magnitude, and whose docstring explains exactly this. Not a bug.

Testing after the fixes

New tests for each defect: Nm vs nm, unit names not being chopped into letters, dimensionless ratios keeping their identity, and multi-factor units. The old test asserting kg/kg -> "1" was updated, since that is the behaviour deliberately changed.

pytest lib/test/unit_tests: 171 passed, 31 subtests in a full environment; 80 passed, 68 skipped in a virtualenv holding only PyYAML and pytest, the way CI builds it. darker --check --revision main clean. pylint on adf_units.py 10.00/10.

End to end, unchanged from before the fixes: the three AMWG tables are still byte-identical to main, a plotting re-run still rewrites 0 files with every variable reported as already done, and the thirty meridional plots are byte-identical across the change. The defaults sweep still finds 0 variables whose conversion would be wrongly skipped.

Still not verified

tem at runtime and meridional_mean under compare_obs: true — neither has data staged on this machine. The tem fix is a one-word name correction that py_compile and an AST check confirm resolves, but the surrounding apply_conversion call has not been executed against real TEM files.