Compare units through UDUNITS instead of a parser of our own - #34
Merged
Merged
Conversation
The checker's small units parser compared base names literally, so 'kelvin' was not 'K' and 'second' was not 's', and PISM's files, whose units strings come from its own UDUNITS, failed on both (discussion ismip#46). An alias table would fix those two and wait for the next: seconds, sec, metre, meters, degK, watts. CF says a units attribute must be "a string that can be recognized by the UDUNITS package", so the comparison is now UDUNITS's own, through cf-units. Equal means the same dimension at the same scale with the same offset, so 'm yr-1' is still not 'm s-1', 'kPa' is not 'Pa' and 'degC' is not 'K'; and a string UDUNITS cannot parse is reported as such rather than as the wrong unit. Every row of the old test table gives the same answer, except 'kg/(m2 s)', which the parser gave up on and UDUNITS understands. A test checks that the request's own units strings all parse, since a typo there would fail every file of a variable. cf-units >=3.3 is the floor: the first release built against numpy 2. It goes into pyproject.toml, both environment files and the dependency table, and needs adding to the feedstock recipe at release. Bump the version to 0.5.0 for the new dependency and the changed check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
From https://github.com/orgs/ismip/discussions/46#discussioncomment-18483514: PISM writes
kg m^-2 second^-1andkelvin, and the checker rejects both. Its units parser compares base names literally, sosecond ≠ sandkelvin ≠ K. An alias table would fix those two and wait for the next (seconds,sec,metre,meters,degK,watts, ...); PISM's strings come from its own UDUNITS, so it can emit any name UDUNITS knows.CF defines a valid units attribute as "a string that can be recognized by the UDUNITS package," so this PR makes the comparison UDUNITS's own, through
cf-units(SciTools' UDUNITS-2 binding)._units_matchbecomes six lines and the ~90-line parser goes.Semantics, verified against 3.3.1:
m yr-1is still notm s-1,kPais notPa,degCis notK,percentis not1. So the range check's "units attribute that lies" case is unaffected.test_units_matchtable gives the same answer exceptkg/(m2 s), which the parser gave up on and UDUNITS understands.M) is now reported as "not one UDUNITS recognizes" rather than as the wrong unit, which would send the modeler looking for a factor.unitsstring in the shipped data request parses, since a typo there would fail every file of a variable.Dependency:
cf-units >=3.3,<4(3.3 is the first release built against numpy 2). Added topyproject.toml,isschecker_env.yml,ci/isschecker_env_floor.ymland the dependency table in the developer docs. It's on conda-forge for linux-64/aarch64, osx-64/arm64 and win-64, with PyPI wheels. At release, the feedstock'srun:requirements needcf-units >=3.3,<4added, as the releasing guide describes for a dependency change.Bumps the version to 0.5.0 for the new dependency and the changed check; one release covers this and #33.
🤖 Generated with Claude Code