Skip to content

Fix unit check#14

Merged
xylar merged 1 commit into
ismip:mainfrom
xylar:fix-units
Jul 25, 2026
Merged

Fix unit check#14
xylar merged 1 commit into
ismip:mainfrom
xylar:fix-units

Conversation

@xylar

@xylar xylar commented Jul 25, 2026

Copy link
Copy Markdown
Member

Compare units by what they mean, not how they are spelled

Fixes #8

The problem

The units check compared the units attribute with the data request as plain strings, so a file carrying kg m^-2 s^-1 was flagged with an error telling the modeler it "should be kg m-2 s-1" — two spellings of the same unit. CF requires only that the attribute be "a string that can be recognized by the UDUNITS package", and UDUNITS recognises both, so this was a false positive. It is not a hypothetical one: PISM writes m^2 precisely because pint does not recognise m2. The data request is itself of two minds about this — it writes m^2 for iareagr and iareafl and m-2/s-1 everywhere else — so the checker was demanding the caret in one place and rejecting it in another.

The fix

_parse_units reduces a units string to a canonical (scale, factors) form, where factors is a sorted tuple of (base unit, exponent) pairs. Comparing those canonical forms makes the check insensitive to everything UDUNITS treats as the same unit: the spelling of exponents (m2, m^2, m**2), the separator between factors (space, ., *, middle dot), / for negative exponents, and the order of the factors. Division follows UDUNITS' left-to-right associativity, so kg/m2 s is kg m-2 s rather than kg m-2 s-1.

The parser is deliberately small, and says so. Parentheses, timestamp offsets such as days since 1850-01-01, and decimal scale factors all return None, and _units_match then falls back to the literal string comparison that was there before. The change can therefore only ever accept more units than it did, never fewer, and nothing the parser does not understand is guessed at. Unit prefixes are not something it knows about: km still fails against m, as it should.

A file spelled exactly as the request still logs the unchanged The unit is correct: <units> line — which is what leaves the golden reference log untouched — while an equivalent spelling logs The unit is correct: m2 (equivalent to the requested m^2), so the difference is recorded rather than passing silently.

Testing

23 new tests, 31 in total, all green. Five run the whole checker over a file whose units attribute has been rewritten to an equivalent spelling (m2, m**2, kg s^-1, kg/s, kg.s-1) and require zero errors; one requires that m^3 where m^2 was asked for is still an error, with its message unchanged. The rest table-test _units_match directly, covering the equivalences above, the cases that must not match (m3 vs m2, km vs m, M vs m, kg/m2 s vs kg m-2 s-1), and the strings the parser declines to interpret.

The golden-log regression test passes unchanged, confirming that no existing, exactly-spelled file changes its log output.

Run locally against the latest environment (python 3.14, xarray 2026.7); CI covers the floor environment as well.

Also updated

The check summary at the top of isschecker/checker.py and the corresponding bullet in README.md, so the documented behavior matches what the checker now does.

The units check was a string comparison against the data request, so a file
carrying `kg m^-2 s^-1` was reported as an error telling the modeler it
"should be kg m-2 s-1" -- two spellings of the same unit.  CF asks only that
the attribute be "a string that can be recognized by the UDUNITS package",
and UDUNITS recognises both, so the error was a false positive.  It is not a
hypothetical one: PISM writes `m^2` precisely because pint does not
recognise `m2`, and the data request itself writes `m^2` for iareagr and
iareafl while writing `m-2`/`s-1` everywhere else, so the checker was
demanding the caret in one place and rejecting it in another.

`_parse_units` reduces a units string to a canonical (scale, factors) form,
where factors is a sorted tuple of (base unit, exponent).  That makes the
comparison insensitive to the spelling of exponents (`m2`, `m^2`, `m**2`),
to the separator between factors (space, `.`, `*`, middle dot), to `/` for
negative exponents, and to the order of the factors -- all of which UDUNITS
treats as the same unit.  Division follows UDUNITS' left-to-right
associativity, so `kg/m2 s` is kg m-2 s rather than kg m-2 s-1.

The parser is deliberately small, and says so: parentheses, timestamp
offsets such as `days since 1850-01-01`, and decimal scale factors return
None, and `_units_match` then falls back to the literal string comparison
that was there before.  So the change can only ever accept more units than
before, never fewer, and nothing it does not understand is guessed at.
Prefixes are not units it knows: `km` still fails against `m`, as it should.

A file spelled exactly as the request still logs the unchanged "The unit is
correct: <units>" line, which is what keeps the golden reference log
untouched; an equivalent spelling logs "(equivalent to the requested ...)"
so the difference is visible rather than silent.

Verified: 23 new tests, 31 in total, green.  Five of them run the whole
checker over a file whose units attribute has been rewritten to an
equivalent spelling (`m2`, `m**2`, `kg s^-1`, `kg/s`, `kg.s-1`) and require
zero errors; one requires that `m^3` where `m^2` was asked for is still an
error, with the message unchanged.  The rest table-test `_units_match`
directly, including the cases that must not match and the ones the parser
declines to interpret.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@xylar xylar added the bug Something isn't working label Jul 25, 2026
@xylar
xylar merged commit 61f0529 into ismip:main Jul 25, 2026
4 checks passed
@xylar
xylar deleted the fix-units branch July 25, 2026 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unit error false positive

1 participant