Add warnings to the compliance checker

[xylar]

Closes #10. Also settles the stopgap PR #16 left behind, and closes the golden-log gap it recorded.

What this is for

The log has promised warnings since it was written and never delivered any: No warnings. under every file's footer and 0 warning(s) detected. in the synthesis were hardcoded strings. Everything else was an error, because there was no other severity in the counting path — each _check_* function wrote " - ERROR: ..." and returned an int that its caller added on, and there was nowhere for a second severity to live.

So the checker could not distinguish "this file is wrong" from "this file is unusual, please look at it". Issue #10 asks for exactly that distinction, quoting @hgoelzer: some of the min/max bounds "are dependent on the forcing, input data and model implementation", so failing a run on them is too strong. The extra-litemp-snapshot check landed as an error in PR #16 for the same reason — not because it was the right severity, but because it was the only one available.

The rule

One rule, now stated in the header comment of checker.py and in the README, so that the next check is classified by citing it rather than by arguing the case again.

ERROR — the file, as written, is unusable for the intended analysis, departs from the protocol in a way that changes the science, or fails the data-hygiene requirements this archive is committing to. That last clause is deliberate: the output will be served to the broader community for analysis for years, so uniformity of encoding is a product requirement rather than a stylistic preference, and "a reader could cope with it" is not grounds for a warning.

WARNING — the file is usable, the science is unaffected, and nothing downstream has to work around it, but it departs from what the request asked for in a way the modeller should look at and may reasonably have intended.

Three consequences keep warnings from quietly becoming errors: they never enter the error count or change a file's verdict, they never affect the exit status, and a check whose failure means the checker could not read something stays an error — a warning never suppresses a later check.

What changed severity

Four findings move, each in its own commit so that any one of them can be reversed in review without disturbing the others.

Snapshot nominal years the experiment does not call for are now a warning. The data request specifies the snapshot years as a minimum set, so over-delivering 3D temperature is not non-compliance. This was argued over, because the annual time axis is treated the other way round — an extra year there is still an error — and the asymmetry is deliberate: the annual axis is pinned end to end by experiments_ismip7.csv, so an extra year means the file does not match the experiment it names, whereas exceeding a stated minimum is a different act.

A snapshot at nominal year 2000 then needs no special case and TOLERATED_SNAPSHOT_YEARS is gone. It was accepted in silence, which told a modeller following the old README nothing at all. Reported as an unrequested snapshot it gets the honest answer: it is here, it was not asked for, it is not held against you. Issue #12 can settle it either way from there.

An unexpected extra variable in a file is now a warning. The requested variable is present and fully checkable either way, and a reader taking it out of the file is unaffected by what sits beside it. What an extra variable says is that the file was probably not written for this submission — worth a look, not a fault.

A time coordinate that is not float32 is now a warning, split from the main-variable dtype check, which stays an error. A float64 data variable is twice the size it should be for the archive and for everyone who has to move it. A time axis is one number per record, so the size argument that governs the data variable simply does not reach it.

What is newly reported

Non-mandatory variables an experiment carries no files for. Nothing reported this before, so a group that meant to submit litemp and lost it in a script got no signal whatsoever. The design work here is in the saying, not the checking, because a deliberate omission is the common case — not every model supports every non-mandatory variable, GIA being the obvious one. So the warning says "carries no files for" rather than "missing", says it once per experiment naming all of them rather than once per variable (which would make a model with a narrow scope look far worse than one that dropped a single file), says out loud that this is expected if the model does not represent them, and stays out of the trailing naming-issues report, which is the part of the log that reads as a list of faults. It is scoped to the selected --variable-list, exactly as the mandatory-variable check is.

An optional not_modelled.txt in the source directory silences that warning for the variables it names — one name per line, # starting a comment so the reason can sit beside the name. Groups already submit a README explaining what they submitted and why some variables are absent, so the checker has no need to ask for that list again, and an explanatory list does not belong on a command line where it would have to be retyped for every run.

Two rules keep it from becoming a way to hide problems, and both are errors rather than silent no-ops. A mandatory variable named in it is still a missing-mandatory error and the claim itself is an error: the list is a statement about optional variables, and a submission cannot opt out of the data request with it. A name that is not in the data request at all is an error too — typo or misunderstanding, both better said plainly than left to be inferred from a warning that did not go away. Whatever was declared is echoed into the log, so the archived record of a run shows what was claimed rather than merely that a warning did not appear. An absent file changes nothing.

The mechanism with no behaviour change

ISMIP7_variable_request.csv gains a range_severity column, read by _load_criteria and honoured by _check_numerical. Every shipped row is error, so nothing about value ranges behaves differently today. This is the substance of issue #10, and the decision is that the classification is per variable rather than global: some variables have bounds a legitimate model can exceed and some do not, and there is no way to tell which from the checker's side.

Switching a variable is then a one-cell, data-only diff that needs no code review and no reasoning about the checker, which is what makes the case-by-case triage cheap enough to actually happen and easy to revise as feedback comes in from groups. A blank cell, an unrecognised value and a missing column all mean error, which is what makes the column safe to add now and fill in gradually. Region granularity (range_severity_ais / range_severity_gris) is a strictly larger version of the same design and can replace the single column later without touching the check.

The one thing that affects existing automation

The checker now exits non-zero when there are errors, and zero when there are only warnings. It always exited 0 before, so nothing could gate on it. Two details beyond the obvious: python -m isschecker discarded main()'s return value and now raises SystemExit, matching the console script; and the paths where the checker gives up — directory not found, no .nc files, a dataset it could not read — return a summary whose error count is zero because nothing was checked, which on an error-based exit status would have exited 0 after checking nothing. A fatal flag tells that apart from a clean run.

This is the last commit in the series precisely because it is the only one that changes how the tool behaves for existing automation, so it can be released, reverted or announced on its own.

How it was done

Severity has to reach both the log line and the counters, and there were roughly sixty call sites writing " - ERROR: ..." and incrementing a local errors in parallel. Threading a second integer through every function and every caller would have doubled that bookkeeping and made every future severity change a two-line edit in two places.

Instead a Reporter owns the log file and the counts, so that the write is the count. Reporters nest: category() scopes findings to one of the reporting categories, and child() opens a sub-total — one file's worth — that still rolls up into its parent, so a file's footer, an experiment's console line and the run-wide synthesis are sub-totals of one counting rather than three additions that have to agree. The _check_* functions no longer return counts at all; they say what they found at the severity they mean, and changing a classification is a one-word edit.

The first two commits are that migration and are behaviour-preserving by construction: if the reporter emits the same strings, the reference log does not move, and the golden-log test proves it. Every commit through the demotions leaves the references untouched except where a change was intended, and each of those regenerates them in the same commit.

Two fixes that came with it

The companion-variable allowlist was too narrow. _allowed_file_variables understood bounds, grid_mapping and coordinates but not cell_measures or ancillary_variables, so a file carrying a cell area its requested variable is normalised by was reported as holding an unexpected variable. That is a false positive, not a soft finding, so it is removed outright rather than demoted — and before any severity change, so the two are not confused.

The "closest requested name" hints were nondeterministic. The candidates came from a set, so two equally close names resolved by iteration order and the same checker over the same files could produce different logs on different runs — which is the one thing this tool is not allowed to do. They are sorted now.

Reporting changes

• Per-file footer — the warning count is real: No warnings. or N warning(s). Please review.. A file with warnings and no errors still reads No errors. Good job ! on the line above.
• Synthesis block — a real warning total with the same per-category breakdown the errors get. The first category is renamed from Mandatory variables to Variable presence, which the non-mandatory warning otherwise contradicts.
• Console — the per-experiment and total summaries mention warnings when there are any, phrased so it cannot be read as a failure.
• run_checker summary dict — gains total_warnings, the per-category warning keys, and fatal.

Testing

tests/test_reporter.py is new: the line formats, the split between severities and categories, and the roll-up from a file's sub-total to the run's total, which is what the footer and the synthesis both rest on.

Every demotion has a test asserting that the error count went down and the warning count went up, which is what would catch an accidental re-promotion. New tests cover the not-submitted warning and its scoping, not_modelled.txt in all four of its cases, range_severity (driven by a synthetic criteria row, so the mechanism and the shipped data are tested independently), and the exit status including both fatal paths.

A second reference log, generated with include_non_mandatory=True, closes the gap PR #16 recorded: litemp is the data request's only x,y,z,t variable and so the only one whose snapshot axis the time checks look at, and it is not mandatory, so the whole snapshot branch had no golden coverage. Neither did the static x,y refgeoid. The existing reference keeps its meaning and is unchanged by that commit.

CI now runs the whole tests/ directory. It named tests/test_compliance_checker.py explicitly, so test_golden_log.py never actually ran there — despite its entire reason for existing being to catch a dependency release inside the supported ranges changing the log, which is what running it at both the floor and the latest environment is for.

Deliberately not here

Filling in the per-variable range severities. The mechanism ships with every variable still an error; deciding which ones move is case-by-case work for after this, as data-only changes.