Act on independent review: unit scale correctness, grid guard, lenient display inference

[hdrake]

An independent reviewer audited the units feature (both #32 and #33) for bugs and
separately tried to write a recipe for their own model using only the docs. This
acts on what was worth acting on. Targets units-handling (#32), since #33 was
merged into it while this was in progress.

Everything here reproduces — each fix landed with the failing case first.

Correctness

The ECCO salt budget's stamped units were wrong by 1000×

ECCO's salt fluxes carry the salinity scale in their own units string
(ADVx_SLT is 1e-3 m3 s-1), and the recipe's compensating
unit_conversion: 0.001 was a bare number, hence dimensionless. So the
1e-3 survived into the composed units while the arithmetic already produced
kg/s:

stamped units : 0.001 kg.s-1
value         : 35.60          <- correct kg/s
read with its own units -> 0.0356 kg/s     # off by 1000

Eight ECCO variables shipped that label. The budgets still closed (both sides
carried the same error), so nothing looked wrong — but the attribute is written
to disk, and it is the thing this feature exists to get right.

The conversion constants now declare the reciprocal scale, so value × units == 1
and the scale cancels:

constants:
  salinity_scale:      {value: 0.001, units: "1e3"}     # cancels the input's 1e-3
  grams_to_kilograms:  {value: 0.001, units: "kg g-1"}  # SFLUX/oceSPtnd are g m-2 s-1

ECCO salt now stamps kg.s-1. Values are byte-identical — closure residuals
unchanged (mass 2.590e-04, heat 5.111e-06, salt 1.206e-04, exactly as before).

assert_unit_consistency now checks magnitude, not just dimension

is_convertible is blind to a numeric scale, so it passed the above. It now
requires the inferred units to equal the declared ones, and reports a
scale-mismatch naming the factor and the fix. Both shipped models reconcile
exactly under the stricter check.

This reverses a decision documented at length in #32 ("why strict checks
dimension, not exact units"). The motivating case there was salinity's 1e-3 —
which is better handled by making the recipe cancel the scale than by declining
to look at it. Worth your call; it is one predicate (same_scale) if you'd
rather keep the looser check.

lateral_divergence without a grid raised AttributeError

A regression from #33: the rechunking reads self.grid.axes before the guarded
helper is reached.

before: AttributeError: 'NoneType' object has no attribute 'axes'
after:  ValueError: Input `data` must be an `xgcm.Grid` instance when using
        `lateral_divergence` operations.

Both grid-needing operations now share one _require_grid, and the divergence
has the grid-guard test difference already had.

show_recipe blanked the units above any missing diagnostic

Static inference had no notion of "the evaluator will drop this operand", so one
absent diagnostic poisoned every ancestor — on the raw ECCO dataset, all three
rhs roots, the most useful nodes in the display:

raw ECCO dataset       before    after
  mass/rhs             None      kg.s-1
  heat/rhs             None      W
  salt/rhs             None      kg.s-1
  mass/lhs             None      None     <- correct: genuinely won't be built

infer_units(node, lookup, available=) now mirrors the run when told which
diagnostics exist: a sum drops what the evaluator drops, a product missing a
factor is dropped whole, an optional subtree stops blanking its parent. A term
that genuinely will not be built stays blank — leniency, not optimism.

Smaller fixes

constants entry with no value → bare KeyError	BudgetParseError naming the path
Budget named constants → same KeyError	says the key is reserved
reciprocal operand unannotated in the HTML tree	annotated (it was correct in the ASCII one)
UnitError preempted MissingDiagnosticError	missing diagnostics reported first — the absent input causes the unknown units
units: on a term → "you may be missing an enclosing product"	its own message; that hint pointed at the wrong fix
Rechunking dask-ified unchunked data	no-op on unchunked input, which is already one chunk

Docs

The reviewer got through recipe-writing on the docs alone and was stopped by one
thing: recipes.md never showed how to build an xgcm.Grid. Quickstart says
a grid is needed and to "pass it in exactly the same way", but nothing shows
constructing one — and xgcm ≥ 0.10 renamed boundary= to padding= and removed
periodic=, so every older tutorial they'd find hands them a ValueError. They
had to read tests/conftest.py. There is now a ### The grid section with a
worked snippet and a note about the rename.

Also:

• The old guidance — "salinity-to-mass conversion factors, and the like are
  dimensionless and stay bare" — is what led them into the 1000× bug. It now
  says the opposite, with the worked pattern.
• Documented what the assertion does not check (unmaterialized side roots).
• Corrected the constants-substitution scope: sum/product operands only, not
  "a string operand" (a difference/reciprocal operand is always a diagnostic).
• aggregate() returns metadata beside lhs/rhs, and now that every recipe
  declares units that trips anyone iterating the values. Noted in quickstart.

Verified

• 178 tests pass (was 157), with both example datasets present so the MOM6
  characterization golden runs rather than skips — it still matches.
• assert_unit_consistency=True passes on real MOM6 and real ECCO V4r4 LLC90
  under the stricter magnitude check.
• ECCO closure residuals unchanged to every printed digit.
• Three notebooks re-executed; docs build clean under -W --keep-going.

Reviewer findings deliberately not acted on

• _ALIASES is a whole-string map, so psu m3 s-1 parses as unknown while
  psu aliases to 1. No shipped recipe or example dataset hits it, and
  token-level rewriting of a UDUNITS string is its own small risk. Worth an issue.
• Stamped units are cf-units canonical form (kg.s-1, not kg s-1). Legal
  UDUNITS and consistent, but it does differ from what input datasets write.
  A deliberate choice worth confirming rather than a bug.
• A typo'd constant reference degrades into a missing diagnostic with no hint
  that a similarly-named constant was declared. The evaluator has no access to
  the table by then (references are substituted at parse time), so a good hint
  needs plumbing that did not seem worth it here.

🤖 Generated with Claude Code

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB