From 77332c4d5f37843f449ab6dad907bd3836ef9cd1 Mon Sep 17 00:00:00 2001 From: Juniper Bevensee Date: Tue, 28 Jul 2026 12:29:58 +1200 Subject: [PATCH 1/3] feat(comparison): comparator-as-record registry + trustworthy-comparison method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Promotes, under docs/promotion-and-upstream.md Path A, the generalizable methodology from an audit of one experimental comparison that was invalid four different ways across three attempts, each fix moving the bug rather than removing it: a hardcoded comparator printed by eight scripts and read back out of a job notification; a reimplementation of the baseline (recall 0.636 -> 0.098); the wrong one of two variants, chosen by name (0.731 vs 0.442); and the right one run at library signature defaults instead of its tuned parameters. Every particular is stripped — no study, dataset, species, model, filename or path. What is promoted is the method and the enforcement. docs/trustworthy-comparison.md — the case study and 14 checkable rules. Also covers the other generic failure modes from the same audit: a declared-but-unused `SR = 22050` constant that scripts believed and resampled to, changing true positives 2.1x; a label column that was a frequency in Hz, producing 829 singleton classes over 829 examples while the real 13-family/74-label taxonomy sat in an adjacent file; an optimum on the boundary of the swept range read as evidence of confidence; unseeded torch making the same script produce F1 0.7839 then 0.7675 and overwriting the artifact a published figure cited; and a helper that printed HTTP bodies and exited 0, so a 405 was indistinguishable from success. Generalized: anything that cannot fail loudly will eventually be believed. docs/baseline-registry.md — the four welded fields, the API, the contract a producing script must satisfy, and a "what this still does NOT catch" section. That last part is the most valuable: structure makes disagreement visible, not truthful. docs/results-provenance-checklist.md — what a results JSON must carry to be regenerable from itself. matilde_plugin/engine/comparison.py — stdlib-only, domain-neutral vendoring. compare() raises IncomparableError rather than returning a delta when the split or criterion differ; split identity is a hash of the sorted member list, never a label; Criterion is structured data with prose excluded from equality; run() is bound to the tuned parameters and refuses overrides. tests/test_comparison.py — 39 tests, one per failure, each named for the mistake it reproduces. Suite: 144 -> 183 passed, 7 skipped. The re-scored resolution is stated with its caveats wherever it appears: under one split and one matcher the classical detector beat the CNN (0.801 vs 0.775 validated matcher; 0.762 vs 0.747 IoU-only), both arms oracle-best on test so upper bounds not held-out estimates, and it won despite 22 of 30 test files having leaked into the CNN's training/validation set. Co-Authored-By: Claude Opus 5 --- README.md | 16 + docs/baseline-registry.md | 256 ++++++++ docs/results-provenance-checklist.md | 118 ++++ docs/trustworthy-comparison.md | 342 +++++++++++ hermes-skill/SKILL.md | 31 + matilde_plugin/engine/comparison.py | 866 +++++++++++++++++++++++++++ tests/test_comparison.py | 434 ++++++++++++++ 7 files changed, 2063 insertions(+) create mode 100644 docs/baseline-registry.md create mode 100644 docs/results-provenance-checklist.md create mode 100644 docs/trustworthy-comparison.md create mode 100644 matilde_plugin/engine/comparison.py create mode 100644 tests/test_comparison.py diff --git a/README.md b/README.md index 685da64..35d4e31 100644 --- a/README.md +++ b/README.md @@ -152,10 +152,26 @@ promotion flow. | `matilde_plugin/engine/citations.py` | The verifiable-citations engine (the core; the part we may open-source standalone) | | `matilde_plugin/engine/openneuro.py` | Read-only OpenNeuro/BIDS client — discovery, metadata, files (stdlib-only) | | `matilde_plugin/engine/parsing.py` · `matilde_plugin/engine/cli.py` | BibTeX/DOI ingestion + the `matilde` verify CLI (`python3 -m matilde_plugin.engine.cli`) | +| `matilde_plugin/engine/comparison.py` | Comparison registry + reproducibility guards — makes an invalid A-vs-B comparison **refuse** instead of printing a delta (stdlib-only, domain-neutral) | | `hermes-skill/SKILL.md` | The agent's research methodology | | `docker/SOUL.Matilde.md` | The research-assistant identity | | `tests/` | Offline unit suite + live API integration tests | +## Method docs + +`docs/` holds the methodology this package encodes — each document exists because +something specific went wrong, and says what. + +| Doc | What it covers | +|---|---| +| [trustworthy-comparison.md](docs/trustworthy-comparison.md) | How to establish that two measured numbers are comparable before you subtract them. Case study: one comparison that was invalid four different ways across three attempts, each fix moving the bug. Plus dead constants, label columns that aren't labels, boundary optima, unseeded runs, and silent failure. | +| [baseline-registry.md](docs/baseline-registry.md) | The comparator-as-record pattern — callable + tuned params + split + criterion — and why a registry returning only a *number* prevents one of those four failures and none of the others. Includes an honest list of what it still does not catch. | +| [results-provenance-checklist.md](docs/results-provenance-checklist.md) | What a results file must carry to be reproducible from itself: seed, library versions, git SHA, the split member lists, every data-reduction decision, the matching criterion, and how the operating point was chosen. | +| [golden-validation-recipe.md](docs/golden-validation-recipe.md) | The offline, dependency-free worked validation — the reference shape of a correct finding, and the package's smoke test. | +| [meg-validation-study.md](docs/meg-validation-study.md) · [stateful-study-pipeline.md](docs/stateful-study-pipeline.md) | The bounded-sample validation study and the resumable step pipeline behind it. | +| [privacy-and-visibility.md](docs/privacy-and-visibility.md) · [promotion-and-upstream.md](docs/promotion-and-upstream.md) | The privacy model, the sanitization gate, and how a technique gets promoted out of a private overlay into this package. | +| [onboarding.md](docs/onboarding.md) | Start here if you are new to the package. | + --- *Matilde is private during development. The citation engine is intended for the public diff --git a/docs/baseline-registry.md b/docs/baseline-registry.md new file mode 100644 index 0000000..aa0a415 --- /dev/null +++ b/docs/baseline-registry.md @@ -0,0 +1,256 @@ +# The baseline registry + +A comparator is not a number. It is **four things welded together**, and dropping +any one of them produced a real invalid comparison: + +| field | why it is in the record | +|---|---| +| `fn` | the *validated* callable, imported — never re-written inside a comparison script, and never chosen by matching a function name | +| `params` | the parameters it was **tuned to on the train split**, read out of the results file — not the function's signature defaults | +| `split` | the split it was scored on, identified by a **sha256 of the sorted member list**, plus the ground-truth count implied by `tp + fn` as a second fingerprint | +| `criterion` | how a prediction counts as a match, as a comparable structured record — not prose | + +`compare(a, b)` refuses to produce a delta unless the **split** and the +**criterion** match. That refusal is the whole product. Everything else is +bookkeeping in service of it. + +The case study those four fields come from is +[trustworthy-comparison.md](trustworthy-comparison.md). Read it first; every +docstring in the module refers to it. + +**Implementation:** `matilde_plugin/engine/comparison.py` (stdlib only, +domain-neutral). **Tests:** `tests/test_comparison.py`. + +--- + +## Why not just return a number? + +The obvious design is a lookup table: `get_baseline("classical") -> 0.731`. It +would have prevented the first of the four failures and **none** of the other +three. + +| failure | prevented by a number-returning registry? | +|---|---| +| 1. Hardcoded comparator printed by eight scripts | **Yes** — there would be one place to read it from | +| 2. Baseline reimplemented by hand inside the comparison script | No — the number is right, the arm is a different program | +| 3. Wrong one of two detector variants, chosen by name | No — the number is right, it describes a function that was not run | +| 4. Run at library signature defaults instead of the tuned parameters | No — the number is right, the re-scored arm is handicapped | + +Worse, a number-returning registry makes 2–4 *harder* to catch, because the +comparator now looks authoritative. Every field beyond the number exists because +its absence let a specific comparison go out wrong. + +--- + +## Register an arm + +Preferred — params, metrics, split and criterion all read from one file, one +method key, in one call: + +```python +from matilde_plugin.engine.comparison import register_baseline_from_results +import my_validated_module + +register_baseline_from_results( + "classical", "validation_results.json", + criterion_keys=("iou_threshold", "overlap_threshold"), + method="simple", + fn=my_validated_module.detect, + iou_op=">=", # asserted by the caller — see "operators" below +) +``` + +Nothing above is typed in. The parameters, the per-split metrics, the split's +member list and the matching thresholds are all read from the results file at +registration time. Because the parameters and the metrics come from the *same* +declared method key, they provably describe the same run — which is what closes +failures 3 and 4 together. + +### What the producing script must save + +This is the contract. A results file must carry: + +| key (default name) | what it holds | +|---|---| +| `results_by_method..parameters` | the tuned parameters | +| `results_by_method..aggregate.` | metrics per split role (`test`, `train`, …) | +| `test_items` | the split's **member list** — filenames or ids, not a description | +| `matching_criteria` | the fields named in `criterion_keys` | +| `best_method` | *(optional)* which method key produced the headline number | + +Every key name is a parameter (`methods_key`, `params_key`, `metrics_key`, +`split_key`, `criterion_block_key`, `role`, `n_units_keys`), so the shape maps +onto whatever your producing scripts already write. What is **not** optional is +that the values exist. If a results file saves none of them, the arm cannot be +registered without asserting fields from memory — which is the failure the +registry exists to stop, and the fix belongs in the producing script, not here. + +`n_units_keys` defaults to `("tp", "fn")`: the ground-truth count is *derived* +from the recorded counts, never typed. For a task where the total is not `tp+fn`, +name the keys that sum to it. + +### Manual registration, when the numbers come from elsewhere + +```python +from matilde_plugin.engine.comparison import Criterion, register_baseline + +register_baseline( + name="learned_v4", + fn=None, # scored from a saved model + params={}, + split=test_member_list, # hashed for you + criterion=Criterion(iou_threshold=0.1, overlap_threshold=None, iou_op=">", + note="train.py:206 `if best_iou > 0.1`"), + source="learned_v4_results.json", + metrics={"test": {"f1": 0.775, "tp": 447, "fp": 130, "fn": 52}}, + provenance={"criterion_from": "asserted: train.py:206"}, +) +``` + +Any provenance value starting with `asserted` — or containing `reconstructed` — +is surfaced in `compare()`'s `provenance_warnings`. A registry can *require* a +field it cannot *verify*; the honest response is to make the assertion visible +rather than let it pass as file-derived. + +--- + +## Compare two arms + +```python +res = compare("classical", "learned", metric="f1", split="test") +# {'split_id': '97fd273d86db', 'n_gt_units': 499, +# 'a': {'name': 'classical', 'value': 0.801, 'counts': {...}, 'params': {...}, ...}, +# 'b': {'name': 'learned', 'value': 0.775, 'counts': {...}, ...}, +# 'delta': 0.026, 'provenance_warnings': []} +``` + +Same split, same criterion → the delta is legitimate and both numbers came out +of files. **Check `provenance_warnings`:** a non-empty list means one arm carries +a field a human asserted rather than read from a results file. + +When the arms do not match, `compare()` raises `IncomparableError` — a +`SystemExit` subclass, so an unguarded script dies at the point of the invalid +comparison instead of printing a number that gets quoted later. The message names +what differs and what to do: + +``` +REFUSING TO COMPARE 'classical' against 'learned': the arms are not comparable. + + SPLIT MISMATCH + classical: split_id 97fd273d86db (30 members, 376 GT units; test of validation_results.json) + learned: split_id 2c6db9a353dd (30 members, 499 GT units; test of learned_v4_results.json) + -> 16 member(s) only in classical, 16 only in learned + only in classical: rec_0041 + ... and 13 more only in classical + + CRITERION MISMATCH + classical: iou_op='>=', iou_threshold=0.3, overlap_threshold=0.5 + learned: iou_op='>', iou_threshold=0.1, overlap_threshold=None + -> differs in: iou_op ('>=' vs '>'), iou_threshold (0.3 vs 0.1), + overlap_threshold (0.5 vs None) + + WHAT TO DO — a delta between these is not a smaller or larger number, it has + no meaning: + 1. Choose ONE split. Use its member list, not a description of how it was + generated. + 2. Choose ONE Criterion(...) and score both arms under it. + 3. Re-score BOTH arms — the registered ones via get_baseline().run(...), + which is bound to their tuned parameters. + 4. Register the re-scored arms and compare() those. + Do not report the delta you were about to report. +``` + +Note that **both splits had 30 members**. The count matched; the contents did +not. That is why `split_id` is a hash over the sorted member list and not a +label — "the seed-42 50/50 split" was a true description of both. + +There is a second, independent check. Same member list but a different +ground-truth count (376 vs 499) is refused separately, as +`GROUND-TRUTH COUNT MISMATCH`: identical split, different label extraction. + +--- + +## Re-run an arm + +```python +pred = get_baseline("classical").run(audio) # bound to threshold=0.5, min_dur=10, gap=50 +``` + +`run()` takes no parameter overrides. **An arm with different parameters is a +different arm**: tune it on the train split and `register_baseline()` it under a +new name. Overriding here reproduces failure 4, and binding one method's tuned +parameters to another method's callable raises rather than silently dropping the +ones the signature does not accept. + +The strongest verification available is to re-run the registered callable with +the registered parameters under the registered criterion on the registered split +and confirm it **reproduces the recorded counts exactly**. If it does, the record +is provably the run that produced the number. Make that a test you can run on +demand; it is the only check that closes the gap between "recorded" and "true". + +--- + +## On operators, and prose that is wrong + +`Criterion.from_results()` takes the **numbers** from the results file and the +**operators** from the caller. That split is deliberate, and it comes from a real +discrepancy: the results file declared its criterion as `IoU > 0.3 OR +overlap > 50%` while the code that actually did the matching evaluated +`iou >= threshold or overlap >= threshold`. Parsing the operator out of the +sentence would have encoded the file's own error. + +So the caller passes the operators, having read the scoring code, and the sentence +is kept in `Criterion.note`, which is **excluded from equality**. Free text can +be displayed but can never make two different criteria look equal — nor make two +identical ones look different because someone reworded the description. + +```python +Criterion(iou_threshold=0.3, note="one sentence") == Criterion(iou_threshold=0.3, note="another") +# True — note is not compared + +Criterion(iou_threshold=0.3, overlap_threshold=None) == Criterion(iou_threshold=0.3) +# False — "declared no fallback" and "never said" are different states +``` + +--- + +## What the registry still does NOT catch + +The most useful section in this document. Be aware of all of it before trusting +a green `compare()`. + +1. **A criterion its source file never recorded.** If the producing script wrote + no criterion block, the criterion has to be read out of the scoring code by a + human and asserted. The registry can *require* the field and *refuse a + mismatch*; it cannot verify a value that exists nowhere on disk. It surfaces + the assertion in `provenance_warnings` rather than hiding it — but the real + fix is in the producing script. +2. **A wrong-but-well-formed criterion.** If you register `iou_threshold=0.3` + for an arm that was actually scored at `IoU > 0.1`, both arms agree and + `compare()` proceeds happily. **Structure makes disagreement visible, not + truthful.** +3. **Tuned parameters bound to a callable that happens to accept them.** The + params/callable check is one-directional: it catches a parameter name the + function does not accept, but if the wrong variant accepts every name, the + mismatch passes. Only `register_baseline_from_results()` — which reads the + callable's method key, its parameters and its metrics in one step — closes + that direction. +4. **Matching semantics beyond the declared fields.** A field like `match_order` + is a *declared string*, not a verified behaviour. Two arms can share the + label and still match differently — one taking the first best-scoring match + per prediction in prediction order, the other doing greedy matching over + candidates sorted by length. The criterion record says what someone claimed, + not what the matcher did. +5. **The metrics themselves.** `compare()` reads recorded numbers. Only an + explicit re-run path regenerates them, and only for arms that carry a + callable — an arm scored from a saved model carries none. +6. **A badly chosen split.** `compare(..., split="all")` or `split="train"` is + legal. The registry refuses a *missing* split, not a leaky one. Nothing here + detects that 22 of 30 test files were in the other arm's training set; that + is a fact about how the splits were built, and it belongs in the results + file's provenance block and in the caveats you print beside the number. + +Items 2, 4 and 6 are the ones to hold onto. They are the reason the registry is a +floor and not a guarantee: it removes the failures that come from *losing +information*, and it cannot remove the failures that come from information being +wrong at the source. diff --git a/docs/results-provenance-checklist.md b/docs/results-provenance-checklist.md new file mode 100644 index 0000000..ce6547f --- /dev/null +++ b/docs/results-provenance-checklist.md @@ -0,0 +1,118 @@ +# Results provenance checklist + +**The test:** a results file is finished when a second script — or you, six weeks +later, with the conversation gone — can **regenerate its headline number from the +file alone.** Not "understand roughly what was done." Regenerate. + +Everything below exists because its absence made a real number +non-reproducible. The failures are recorded in +[trustworthy-comparison.md](trustworthy-comparison.md). + +--- + +## The checklist + +A results JSON must carry: + +- [ ] **Seed, and which RNGs it was applied to.** Not `"seed": 42` alone — + `random.seed(42)` fixed only a file-level split while weight init, dropout + and dataloader shuffling drew unseeded, and the same script on the same + data returned F1 0.7839 then 0.7675. Record the list of libraries actually + seeded, so an unseeded library cannot look like a seeded one. +- [ ] **Library versions** for anything whose behaviour affects the number + (numpy, torch, scipy, the model library), plus the Python version. +- [ ] **Git SHA** of the code, and the script filename. A number produced by + uncommitted code should say so. +- [ ] **The exact split member lists** — train, dev, test — or a hash of each. + A *description* is not enough: two runs both described as "seed 42, 50/50" + produced 30-file test splits that differed in 16 files. +- [ ] **The ground-truth unit count per split**, derived (e.g. `tp + fn`), as an + independent fingerprint. It catches "same files, different label + extraction" — which is how a 499-vs-376 mismatch surfaced before anyone + noticed the file lists differed. +- [ ] **Every data-reduction decision**: duration caps, file caps, channel + selection, decimation, resampling, frequency bounds, minimum-duration + filters. One script read the first 180 s of each recording and scored + against full-file annotations, making 39.8% of ground truth unreachable by + construction — an engineering shortcut reported as an algorithmic result. +- [ ] **The preprocessing configuration used at evaluation**, stated explicitly + and asserted identical to the training one. A sample rate that differed + between training and inference changed true positives by 2.1×. +- [ ] **The matching / scoring criterion** as structured fields, with operators + — `iou_threshold`, `overlap_threshold`, `iou_op`, tie-breaking order — not + as a sentence. And know that the sentence can be wrong: one file's prose + said `IoU > 0.3` where the code evaluated `>=`. +- [ ] **How the operating point was chosen.** Which split the threshold was + selected on, the full sweep that was searched, and whether the best value + landed on the boundary of that range. If it was chosen on the reported + set, name the key `test_set_oracle_best` so nobody quotes it as held-out. +- [ ] **Known leakage between splits**, if any, stated as a count. "22 of 30 test + files were in the other arm's training set" belongs beside the number, not + in someone's memory. +- [ ] **The tuned parameters actually passed to each arm** — not the names of + the functions, the values. Re-runs at library signature defaults are + indistinguishable from tuned runs otherwise. +- [ ] **Every diagnostic that was computed**, including the unflattering ones. A + pipeline that computes an agreement figure of 35.8% and reports a 91% + purity number elsewhere has chosen; the choice is the problem. + +## Two rules about the file itself + +- [ ] **Never write a rerun into an existing output path.** Stamp it + (`run_stamped()`). A rerun once overwrote the artifact a published figure + was read from, and the published number then existed nowhere on disk. An + artifact behind a published figure must be immutable. +- [ ] **Keep one results index** — one row per run: script, git SHA, date, + dataset, `n_train`/`n_test`, ground-truth unit count, matching criterion, + threshold-selection procedure, headline metrics. **A run not in the index + does not exist.** Without one, two files claimed F1 = 0.784 and F1 = 0.361 + for the same model on the same data with nothing reconciling them. + +## Assertions that cost nothing + +Put these in the producing script and let them kill the run. Each one would have +caught a real bug: + +- [ ] per-file counts sum to the reported aggregate +- [ ] `tp + fn` is constant across every point of a threshold sweep +- [ ] combined counts equal the sum of the per-class counts +- [ ] the evaluation preprocessing config equals the training config +- [ ] the number of distinct label values is far below the number of examples + (a label column with one value per example is a continuous measurement) +- [ ] every configuration constant declared in the file is read somewhere in it + +--- + +## In code + +`matilde_plugin/engine/comparison.py` supplies the parts that can be collected +automatically: + +```python +from matilde_plugin.engine.comparison import ( + set_all_seeds, provenance_block, run_stamped) + +seed_info = set_all_seeds(42) # returns what it actually seeded, + versions +... +out = run_stamped("results.json") # results.20260728T113000Z.json +json.dump({ + "provenance": provenance_block( + seed_info, # seed, seeded libs, library versions + # ... and the parts only this script knows: + test_items=test_files, + n_gt_units=len(gt), + matching_criterion=criterion.as_dict(), + threshold_selected_on="dev", + threshold_sweep=sweep, + duration_cap_s=None, + resample_hz=None, + train_files=train_files, + ), + "aggregate": metrics, +}, open(out, "w"), indent=2) +``` + +`provenance_block()` fills in the timestamp, script name, git SHA and Python +version, and merges the seed info. Everything else you must pass, because only +the calling script knows it — and the checklist above is the list of what to +pass. diff --git a/docs/trustworthy-comparison.md b/docs/trustworthy-comparison.md new file mode 100644 index 0000000..7b8f0b6 --- /dev/null +++ b/docs/trustworthy-comparison.md @@ -0,0 +1,342 @@ +# Trustworthy experimental comparison + +A measured number is a claim. A **difference** between two measured numbers is +two claims plus an assumption — that the two numbers were produced under the +same conditions — and that assumption is the one nobody checks. + +This document is the generalized method from an audit of one comparison. The +comparison was *"the new model beats the classical baseline."* It was invalid +**four different ways across three attempts by two different agents**, and each +fix moved the bug rather than removing it. Every arithmetic step was correct +every time. No number was fabricated. The subtraction was meaningless. + +The failure modes are not specific to that task. They are: comparator +provenance, protocol matching, split identity, tuning leakage, dead constants, +unseeded runs, and silent failure. The rules at the bottom are checkable in ten +minutes on any comparison you are about to publish. + +Related: [results-provenance-checklist.md](results-provenance-checklist.md) for +what a results file must carry, [baseline-registry.md](baseline-registry.md) for +the code that enforces the refusals. + +--- + +## The case: one comparison, invalid four ways + +The setting is generic — an event-detection task, scored as precision / recall / +F1 against annotated ground truth, with a **classical signal-processing +detector** as the baseline arm and a **small CNN** as the new arm. A detection +counts as a true positive if it matches a ground-truth event under some +overlap criterion. + +### Attempt 1 — the comparator was a hardcoded string + +The comparison script printed: + +```python +print(f"Baseline: P=0.860, R=0.636, F1=0.731") +``` + +An **f-string with no interpolation**. It printed identically on every run +regardless of data, parameters, or split. It sat in eight scripts. It went to +stdout, stdout went into a job-completion notification, and the notification was +read back as a measurement — producing the sentence "the CNN still beats the +baseline" in a single turn with zero files opened. + +The two numbers were not comparable. The new arm was scored at `IoU > 0.1` over +a 499-event split; that baseline figure came from `IoU >= 0.3 OR overlap > 50%` +over a **376-event** split. Both splits had exactly 30 files. Both scripts said +`random.seed(42)` and "50/50 split". One sorted the filenames before shuffling +and the other did not, so 16 of the 30 files differed. + +> **Nothing in the pipeline was positioned to notice.** The criterion travelled +> as a sentence — `"IoU > 0.1"` and `"IoU >= 0.3 OR overlap > 50%"` are both +> non-empty strings, both truthy, and no code path compared them. The split +> travelled as a *description*, which two different splits both satisfied. + +### Attempt 2 — the fix reimplemented the baseline + +Told the comparator was hardcoded, the next attempt removed the constant and +re-wrote the baseline detector **inside the comparison script** instead of +importing the validated one. + +Recall fell **0.636 → 0.098** (F1 0.731 → 0.170) and the new arm "won" by ++0.285. The number was now genuinely measured, and more wrong than before. A +hardcoded constant is at least a *record* of something that once ran; a fresh +reimplementation is a new, unvalidated program wearing the old one's name. + +### Attempt 3 — the next fix imported the wrong function + +Told to import rather than reimplement, the next attempt imported the baseline +module — and picked the wrong one of its two detector variants. It chose by +**name**: the variant whose name looked canonical. The results file recorded +`best_method: "simple"`, and the 0.731 figure came from that one. The variant +that got imported scores **0.442** on the same split. + +The provenance of 0.731 was quoted while a different function was run. + +### Attempt 4 — and ran it at library defaults + +The original run had grid-searched the baseline on the **train** split and saved +the winners: `threshold=0.5, min_duration_ms=10, merge_gap_ms=50`. The re-runs +called the function positionally — `detector(data, sr)` — and silently got the +signature defaults: `0.7 / 30 / 20`. + +The baseline arm was handicapped before a single event was matched. And because +the original runner filtered unrecognised keyword arguments out with a +`co_varnames` comprehension, passing one variant's tuned parameters to the +other's function *dropped two of them without a word*. + +### What the valid comparison actually said + +Re-scored properly — one split, one criterion, both arms — the classical +detector wins: + +| Matching criterion | Classical detector | CNN | Δ | +|---|---|---|---| +| Validated matcher (IoU ≥ 0.3 **or** overlap ≥ 0.5) | **0.801** @ threshold 0.4 | 0.775 @ threshold 0.2 | −0.026 | +| IoU ≥ 0.3 only | **0.762** | 0.747 | −0.014 | + +**State these caveats wherever you state those numbers:** + +- **Both arms are oracle-best** — each arm's operating point was chosen by + maximising F1 *on the test set*. These are upper bounds, not held-out + estimates. (See rule 4.) +- **22 of the 30 test files had leaked** into the CNN's training or validation + set. Only 8 were genuinely held out for that arm. The CNN's number is + therefore inflated — which makes the classical detector's win *stronger* + evidence, not weaker, but it is not a clean measurement of either arm. +- The margins (0.026 and 0.014) are small at this sample size. They are not + claimed as significant; they are stated as *the classical baseline was never + beaten*, which is the honest form of the finding. + +The publishable result is a negative one: **the classical detector still beats +the CNN.** That is a real finding, and it took four invalid comparisons to +reach it. + +--- + +## Why every fix moved the bug + +Each attempt fixed the *symptom named in the last correction* and left the +structure that produced it intact. + +| Attempt | What was fixed | What was still true | +|---|---|---| +| 1 | — | comparator was a string; split and criterion were prose | +| 2 | the constant was removed | the callable was not the validated one | +| 3 | the callable was imported | it was the wrong callable, chosen by name | +| 4 | the right callable | run at parameters nobody tuned | + +The structural cause is one sentence: **the comparator was never represented as +data.** It existed as a number, then as a function name, then as a call — never +as a record carrying its own provenance. So each fix had to re-derive, from +memory or from a name, whatever the previous fix had lost. + +The fix that finally held was to make the comparator a **record of four welded +fields** — the callable, the parameters it was tuned to, the split it was scored +on, and the criterion it was scored under — and to make the subtraction *refuse* +unless the split and the criterion match. See +[baseline-registry.md](baseline-registry.md). + +A registry that returned only a **number** would have prevented attempt 1 and +none of the others. That is the whole argument for four fields instead of one. + +--- + +## Five more failure modes from the same audit + +Each is generic, and each has a mechanism you can check for. + +### A declared-but-unused constant is a live hazard + +A training script declared `SR = 22050` at the top and **never referenced it +again** — the name appears exactly once in the file. The model was trained on +audio at its native 48 kHz; the sample rate came from the file reader, not the +constant. + +Later scripts read that constant as a statement of intent and helpfully +resampled their input to 22050 before inference. The model got input it had +never seen. Same model, same 499-event split, same matching function, same +score threshold of 0.10: + +| Input | True positives | F1 | +|---|---|---| +| resampled to 22050 Hz | 215 | 0.539 | +| native 48 kHz | **449** | **0.766** | + +A **2.1× difference in true positives**, from a constant that did nothing — +holding the split, the matcher and the score threshold fixed, so the resampling +is the only difference. Across the scripts that reported on this model, its F1 +appeared as low as 0.457 and as high as 0.766, and every one of those numbers was +correctly computed. + +> **Rule:** a constant that is declared and never read is not documentation, it +> is a false claim about the pipeline. Grep every configuration constant for a +> second occurrence. If there is none, delete it or use it — never leave it for +> the next reader to believe. + +### A label column that was not a label + +Four scripts used column 3 of the annotation rows as the class label. Column 3 +was **a frequency in Hz** — the row is `[id, start_ms, end_ms, f_mid, f_min, +f_max, …]`. The result was **829 distinct "classes" over 829 examples**: an +830-way softmax with one example each, and a `labels` field in every results +file that was a list of frequencies presented as a taxonomy. + +The real taxonomy was **one file away**, in an adjacent annotation file in the +same directory: 13 families and 74 labels, with a usable class distribution. + +> **Rule:** before using a column as a label, print its distinct-value count and +> its five most common values. **If the number of unique values is close to the +> number of examples, it is a continuous measurement, not a class.** Make that a +> hard guard, not a habit. And when a dataset ships a companion file next to the +> one you parsed, open it. + +### An optimum on the boundary of a sweep + +The reported best operating point was `threshold = 0.3`. It was also the +**lowest value swept**. An optimum sitting on the edge of the search range means +the search never located the optimum — the number is the boundary of where you +stopped looking. + +It was then read as *"the model is confident about a lot of boundaries."* That +inverts the meaning. Needing a *low* threshold to maximise F1 means the metric +only peaks once low-confidence predictions are accepted, which is evidence of +weak calibration, not strength. + +> **Rule:** if the best value is at either end of the swept range, extend the +> range and re-run before quoting the number. `assert_sweep_interior()` refuses +> to report it. + +### Unseeded stochasticity + +`random.seed(42)` was called — and fixed only the file-level split. Weight +initialisation, dropout, and dataloader shuffling all drew from RNGs nothing +seeded. The same script on the same data produced **F1 0.7839, then 0.7675**. +The rerun also wrote to the same fixed output filename, so it *overwrote the +artifact the published figure had been read from*. The published number then +existed nowhere on disk. + +The discrepancy was explained as "a different random train/test split." That was +checkable and false: the seed call precedes the split, so the split is +deterministic by construction, and `tp + fn = 499` in **both** runs — the +identical test set. The explanation reached for the one component that *was* +controlled and missed the one that wasn't. + +> **Rule:** seed every RNG you import, record which ones you seeded and their +> versions, and never write a rerun into an existing output filename. And when +> you explain a discrepancy, **name the check that would falsify your +> explanation, and run it.** If you cannot think of one, you are telling a +> story, not diagnosing. + +### Anything that cannot fail loudly will eventually be believed + +A helper script printed the HTTP response body of each request and exited `0` +regardless of status. A `405 Method Not Allowed` was therefore indistinguishable +from a success: same shape of output, same exit code. A confident, detailed, and +entirely wrong root-cause analysis was written from that symptom. + +This generalizes past HTTP. A pipeline stage that catches broadly and continues, +a matcher that returns an empty list when its input is malformed, a plot that +renders a legend for a series it does not contain, a validation whose refutation +branch is mathematically unreachable — all of these produce output that *looks +like* a result. Given time, the output is quoted. + +> **Rule:** every stage must have a reachable failure. Check the status, not the +> body. Exit non-zero. Assert the free invariants and let them kill the run: +> per-file counts sum to the aggregate; `tp + fn` is constant across a threshold +> sweep; combined counts equal the sum of the per-class counts; the evaluation +> preprocessing configuration is identical to the training one. One assertion +> would have caught each bug above. + +--- + +## The rules + +1. **A number that arrives as text is a claim, not evidence.** Before quoting + any comparator, open the file that produced it. Notification bodies, chat + logs, stdout, and your own recall are leads. +2. **Never hardcode a comparator.** Load it from the results file that produced + it. An f-string with no interpolation is the tell. +3. **Import the validated implementation; never reimplement it in the + comparison script.** A reimplementation is a new program with an old name. +4. **Ask the file which method produced the number** — the results file's own + `best_method`, not whichever function name looks canonical. +5. **Run a comparator at the parameters it was tuned to.** Positional calls + silently pick up signature defaults. An arm run at different parameters is a + different arm and needs its own tuning on the train split. +6. **Identify a split by its contents, never by a label.** Hash the sorted + member list. "The seed-42 50/50 split" described two different splits with + the same size. Carry a second, independent fingerprint too — the ground-truth + count implied by `tp + fn`, which catches "same files, different label + extraction." +7. **Never select a hyperparameter on the set you report.** `max(sweep, key=f1)` + over a test-set sweep is an upper bound, not a result. Use three-way + train/dev/test. If a rerun is not worth it, **rename the key + `test_set_oracle_best`** so nobody quotes it as held-out. +8. **Represent the matching criterion as comparable data, not prose.** Two + criterion sentences never compare unequal. Two structured records do. Keep + the prose in a field that is excluded from equality — and note that the prose + may itself be wrong: in this case the results file said `IoU > 0.3` where the + scoring code evaluated `>=`. Take the numbers from the file and the operators + from whoever read the code. +9. **Write the comparability check out loud in the report,** and refuse to print + the comparison if any row fails: + + | | Arm A | Arm B | + |---|---|---| + | Split (content hash) | | | + | Ground-truth unit count | | | + | Evaluation function | | | + | Matching criterion | | | + | How the operating point was chosen | | | + | Preprocessing (sample rate, caps, decimation) | | | + +10. **Truncate ground truth whenever you truncate input.** One script read the + first 180 s of each recording and scored against full-file annotations: 39.8% + of ground-truth events were unreachable by construction, capping recall at + 0.602 while 0.312 was reported as an algorithmic result. Record every + data-reduction decision in the results file. +11. **Publish the diagnostic you computed.** A pipeline that computes a + seed-label agreement of 35.8%, hedges it correctly in its own print + statement, and then reports a 91% purity figure elsewhere has chosen the + flattering number. That choice is the misconduct, not the low number. +12. **Report negative results.** "The classical baseline still beats our model" + is a finding. A results file recording zero predictions and F1 = 0.0, and a + docstring explaining why an approach was abandoned, are among the most + valuable artifacts a project produces. +13. **Validate the comparison before you calibrate the number.** This is the + subtle one. A report can be *excellent in form* — recommending a range over + a point estimate, flagging that 1.7 points at n=30 is within normal + variation, offering careful language for a meeting — while the comparison + underneath is invalid. The hedging was about one arm's variance; the claim + that needed hedging was "beats the baseline," which no amount of + range-quoting repairs. + + **Calibration language raises the credibility of whatever it is attached + to.** That makes attaching it to an unchecked claim *more* dangerous than + stating the claim bluntly, because a reader takes the hedging as evidence + that the comparison was scrutinised. Ask: *if I am wrong here, is it because + my number is noisy, or because my comparison is meaningless?* Only the first + is fixed by a range. +14. **Audit the work you are currently doing, not just work you are asked to + review.** The one-turn rule: before reporting a favourable result, spend one + turn actively trying to break it. What confound would produce this number? + What would I see if the labels were misaligned? Does the sample count + reconcile with the previous run? Rigor applied to your own in-flight work is + the only rigor that prevents anything. + +--- + +## Where this is implemented + +- `matilde_plugin/engine/comparison.py` — the registry and the guards. + `compare()` raises rather than returning a delta when the arms are not + comparable; `assert_sweep_interior()` refuses a boundary optimum; + `set_all_seeds()`, `run_stamped()` and `provenance_block()` cover rules 6, 7 + and 11 of the [provenance checklist](results-provenance-checklist.md). +- `tests/test_comparison.py` — one test per failure, each named for the mistake + it reproduces. +- [baseline-registry.md](baseline-registry.md) — the API, and an honest list of + what it still does not catch. diff --git a/hermes-skill/SKILL.md b/hermes-skill/SKILL.md index e4cbd36..71981d9 100644 --- a/hermes-skill/SKILL.md +++ b/hermes-skill/SKILL.md @@ -163,6 +163,37 @@ a fully worked, dependency-free example: a planted in-window M100 that yields the package's smoke test; imitate its shape when judging a real result. See `docs/golden-validation-recipe.md`. +### Before you report that A beats B + +A difference between two measured numbers is two claims plus an assumption — that +both were produced under the same conditions — and that assumption is the one +nobody checks. Before printing any comparison, confirm out loud that both arms +share the same **split** (by its member list, not by a description of how it was +generated), the same **evaluation function and matching criterion**, the same +**preprocessing**, and the same **tuning protocol**. If any of those differ, +refuse to print the comparison and say which one differs. + +Three rules that follow, each from a real failure: + +- **A number that arrives as text is a claim, not evidence.** Comparator figures + from a notification, a log, a chat message, or your own recall must be traced + to the file that produced them before being quoted. A hardcoded + `print(f"Baseline: F1=0.731")` once reached a completion notification and was + read back as a measurement. +- **Never select an operating point on the set you report.** `max(sweep, key=f1)` + over a test-set sweep is an upper bound. Say so, or name the field + `test_set_oracle_best`. And if the best value sits at the edge of the swept + range, the optimum was never found — extend the range. +- **Validate the comparison before you calibrate the number.** Hedging language + raises the credibility of whatever it is attached to, so attaching it to an + unchecked comparison is worse than stating the claim bluntly. Ask whether being + wrong here would mean your number is noisy or your comparison is meaningless — + only the first is fixed by quoting a range. + +Full case study and rules: `docs/trustworthy-comparison.md`. Results-file +requirements: `docs/results-provenance-checklist.md`. Enforcement in code: +`matilde_plugin/engine/comparison.py`. + ## Iteration Pattern 1. **Gather** candidate sources diff --git a/matilde_plugin/engine/comparison.py b/matilde_plugin/engine/comparison.py new file mode 100644 index 0000000..ec738cd --- /dev/null +++ b/matilde_plugin/engine/comparison.py @@ -0,0 +1,866 @@ +"""Make an experimental comparison refusable. + +Stdlib only. Domain-neutral. This is the generalized form of a module written +after one comparison — "the new model beats the classical baseline" — turned out +to be invalid four separate ways across three attempts, each fix moving the bug +rather than removing it. `docs/trustworthy-comparison.md` is the case study; +`docs/baseline-registry.md` is the operator walkthrough. + +The single idea: **a comparator is not a number.** It is a callable, the +parameters that callable was tuned to, the split it was scored on, and the +criterion used to score it. Drop any one of those four and you get one of the +four failures. So they are kept welded together here, and `compare()` refuses to +subtract two of them unless the split and the criterion agree. + +A registry that returned only a *number* would have prevented the first failure +and none of the others. That is why the record has four fields, not one. + +Nothing in this module knows what your task is. You supply the criterion's +fields, the callable, and the keys your results files use. What the module owns +is the refusal. +""" +from __future__ import annotations + +import hashlib +import json +import os +import random +import subprocess +import sys +from dataclasses import dataclass, field, replace as _dc_replace +from datetime import datetime, timezone +from typing import Any, Callable, Iterable, Mapping, Sequence + +__all__ = [ + "IncomparableError", + "Criterion", + "Split", + "Baseline", + "split_id_of", + "n_units_from_counts", + "register_baseline", + "register_baseline_from_results", + "get_baseline", + "list_baselines", + "clear_registry", + "compare", + "assert_sweep_interior", + "set_all_seeds", + "run_stamped", + "provenance_block", +] + + +class IncomparableError(SystemExit): + """Two arms were asked for a delta without sharing a split and a criterion. + + Subclasses `SystemExit` on purpose. An unguarded script should die loudly at + the point of the invalid comparison rather than print a number that will be + quoted later — the original failure reached a job-completion notification and + was read back as a measurement. Tests can still catch it. + """ + + +# --------------------------------------------------------------------------- +# Split identity +# --------------------------------------------------------------------------- + +def split_id_of(items: Iterable[str], digest_len: int = 12) -> str: + """Content-derived id for a split: short sha256 over the sorted members. + + THE INCIDENT THIS PREVENTS: two scripts both said `random.seed(42)` and + "50/50 split", and both produced exactly 30 test files — one totalling 499 + ground-truth units, the other 376. One sorted the filenames before shuffling + and the other did not, so 16 of the 30 differed. The *label* "the seed-42 + 50/50 split" compared equal. The splits did not. + + So a split is identified by its contents, never by a name a human chose. Two + runs that describe their split identically but shuffled differently get + different ids here, and `compare()` then refuses them. + """ + if isinstance(items, (str, bytes)): + raise TypeError( + "split_id_of() wants the collection of split members, not a single " + "string. A split id must be derived from split CONTENTS — passing a " + "label is the failure this function exists to prevent.") + members = sorted(items) + if not members: + raise ValueError("refusing to hash an empty split") + return hashlib.sha256("\n".join(members).encode()).hexdigest()[:digest_len] + + +def n_units_from_counts(block: Mapping | None, + keys: Sequence[str] = ("tp", "fn")) -> int | None: + """Ground-truth unit count implied by recorded counts, e.g. tp + fn. + + Derived, never typed. This is the second, independent fingerprint on a + split: the same member list can still yield a different ground-truth count + if the label extraction changed. It is what first exposed the failure above + — 499 against 376 — before anyone noticed the file lists differed. + + Pass the keys that sum to the ground-truth total for your task. + """ + if not block: + return None + total = 0 + for k in keys: + v = block.get(k) + if v is None: + return None + total += int(v) + return total + + +# --------------------------------------------------------------------------- +# Criterion +# --------------------------------------------------------------------------- + +class Criterion: + """A scoring criterion as comparable data, not prose. + + THE INCIDENT THIS PREVENTS: the criterion travelled as a sentence. + `"IoU > 0.1"` and `"IoU >= 0.3 OR overlap > 50%, greedy by segment length"` + are both non-empty strings, both truthy, and no code path compared them — so + an arm scored one way and an arm scored the other way were subtracted and + the difference was published. + + Fields are whatever your task needs; this module never inspects them: + + Criterion(iou_threshold=0.3, overlap_threshold=0.5, iou_op=">=", + match_order="greedy_by_length", + note="IoU > 0.3 OR overlap > 50%") + + `note` is reserved and **excluded from equality**, so free text can be + carried for humans without ever making two different criteria look the same + — or two identical ones look different because someone reworded the + sentence. + + A field set to `None` is not the same as a field absent. `overlap_threshold=None` + means "no overlap fallback", which is a genuinely different criterion from + the same IoU threshold *with* a fallback, and compares unequal here. + """ + + __slots__ = ("_fields", "note") + + def __init__(self, note: str = "", **fields: Any) -> None: + if not fields: + raise ValueError( + "Criterion() needs at least one field. An empty criterion " + "compares equal to every other empty criterion, which is the " + "free-text failure with extra steps.") + object.__setattr__(self, "_fields", tuple(sorted(fields.items(), + key=lambda kv: kv[0]))) + object.__setattr__(self, "note", note) + + def __setattr__(self, name: str, value: Any) -> None: + raise AttributeError("Criterion is immutable") + + @property + def fields(self) -> dict: + return dict(self._fields) + + def get(self, key: str, default: Any = None) -> Any: + return self.fields.get(key, default) + + def __eq__(self, other: object) -> bool: + if not isinstance(other, Criterion): + return NotImplemented + return self._fields == other._fields + + def __hash__(self) -> int: + return hash(self._fields) + + def __str__(self) -> str: + return ", ".join(f"{k}={v!r}" for k, v in self._fields) + + def __repr__(self) -> str: + return f"Criterion({self})" + + def differences(self, other: "Criterion") -> list[tuple[str, Any, Any]]: + """Field-by-field diff, so a refusal can say WHICH part disagrees. + + A key present in one and absent in the other is reported with the + sentinel string `""` rather than `None`, because "declared None" + and "never declared" are different states and conflating them is how a + missing overlap fallback passes for a declared one. + """ + mine, theirs = self.fields, other.fields + out = [] + for k in sorted(set(mine) | set(theirs)): + a = mine.get(k, "") if k in mine else "" + b = theirs.get(k, "") if k in theirs else "" + if a != b: + out.append((k, a, b)) + return out + + def as_dict(self) -> dict: + d = self.fields + d["note"] = self.note + return d + + @classmethod + def from_results(cls, source: str | Mapping, keys: Sequence[str], + block_key: str = "matching_criteria", + note_key: str = "method", + **asserted: Any) -> "Criterion": + """Read named criterion values out of a results file. + + Takes the values named in `keys` from the file, and any `**asserted` + fields from the caller, who is expected to have read the scoring code. + That split is deliberate. In the original incident the results file said + `method: "IoU > 0.3 OR overlap > 50%"` while the code that did the + matching evaluated `iou >= iou_threshold or overlap >= overlap_threshold` + — the prose was off by a boundary condition. Parsing the operator out of + that sentence would have encoded the file's own error. So: numbers from + the file, operators from the caller, and the sentence kept in `note` + where it cannot affect equality. + + Refuses when a key in `keys` is absent. A results file that does not + record how it scored cannot be an arm in a comparison — the criterion + would have to be asserted from memory, which is the first failure. + """ + if isinstance(source, Mapping): + block = dict(source) + origin = "" + else: + with open(source) as fh: + block = dict(json.load(fh).get(block_key) or {}) + origin = os.path.basename(source) + + missing = [k for k in keys if k not in block] + if missing: + raise IncomparableError( + f"\nREFUSING TO BUILD A CRITERION: {origin} has no " + f"{block_key}.{missing[0]!r}" + + (f" (also missing: {missing[1:]})" if len(missing) > 1 else "") + + ".\n" + f" A results file that does not record how it scored cannot be " + f"an arm in a comparison — the criterion would have to be " + f"asserted from memory, which is the failure this exists to " + f"stop.\n" + f" Either add the field to the producing script, or pass an " + f"explicit Criterion(...) and record " + f"provenance={{'criterion_from': 'asserted::'}} so " + f"compare() can surface it.\n") + + fields = {k: block[k] for k in keys} + fields.update(asserted) + return cls(note=str(block.get(note_key) or ""), **fields) + + +# --------------------------------------------------------------------------- +# Split +# --------------------------------------------------------------------------- + +@dataclass(frozen=True) +class Split: + """A split identified by its contents, with the member list kept for diffs. + + `items` is retained (not just the hash) so a refusal can say *which* members + differ. When only an id is available the list is empty and the error says + the diff is unavailable rather than pretending the splits are close. + + `n_units` is the second, independent fingerprint — see `n_units_from_counts`. + """ + + split_id: str + items: tuple[str, ...] = () + n_units: int | None = None + role: str = "test" + source: str = "" + provenance: str = "recorded" + + @classmethod + def from_items(cls, items: Sequence[str], n_units: int | None = None, + role: str = "test", source: str = "", + provenance: str = "recorded") -> "Split": + members = tuple(sorted(items)) + return cls(split_id=split_id_of(members), items=members, + n_units=n_units, role=role, source=source, + provenance=provenance) + + def diff(self, other: "Split") -> tuple[list[str], list[str]]: + return (sorted(set(self.items) - set(other.items)), + sorted(set(other.items) - set(self.items))) + + def describe(self) -> str: + n = f"{len(self.items)} members" if self.items else "member list unavailable" + units = "" if self.n_units is None else f", {self.n_units} GT units" + return f"{self.split_id} ({n}{units}; {self.role} of {self.source})" + + +# --------------------------------------------------------------------------- +# Baseline +# --------------------------------------------------------------------------- + +@dataclass(frozen=True) +class Baseline: + """An arm you can actually re-run: callable + tuned params + split + criterion. + + "Baseline" here means any arm of the comparison, including the new model. + Every field is present because leaving it out caused a real invalid + comparison: + + `fn` — one attempt re-wrote the baseline by hand inside the + comparison script (recall 0.636 -> 0.098) and the next + imported the wrong one of two variants (0.731 vs 0.442). + The validated callable now travels with the record, so + there is nothing to re-derive from a name. + `params` — one attempt called the callable with library signature + defaults instead of the parameters it had been tuned to. + `split` — the published comparison put a 499-unit split against a + 376-unit one. + `criterion` — and scored one arm at IoU > 0.1, the other at + IoU >= 0.3 OR overlap > 50%. + `metrics` — read from `source`, so a number can never be typed in. + `provenance` — records, per field, whether it came from the results file + or was asserted by a human. `compare()` surfaces the + asserted ones, because a registry can require a field and + still not verify it. + """ + + name: str + fn: Callable | None + params: Mapping[str, Any] + split: Split + criterion: Criterion + source: str + metrics: Mapping[str, Mapping[str, Any]] = field(default_factory=dict) + fn_ref: str = "" + provenance: Mapping[str, str] = field(default_factory=dict) + + @property + def split_id(self) -> str: + return self.split.split_id + + def metric(self, key: str = "f1", split: str | None = None) -> float: + """Fetch a recorded metric, or fail naming what the file actually has.""" + split = split or self.split.role + block = self.metrics.get(split) + if block is None: + raise IncomparableError( + f"\nREFUSING: arm {self.name!r} has no {split!r} metrics " + f"recorded (has: {sorted(self.metrics) or 'nothing'}).\n" + f" Source: {self.source}\n" + f" Do not substitute the 'all' or 'train' figure for a held-out " + f"one — that is how a train-split score gets quoted as test.\n") + if key not in block: + raise IncomparableError( + f"\nREFUSING: arm {self.name!r} has no {key!r} in its " + f"{split!r} metrics (has: {sorted(block)}).\n") + return block[key] + + def counts(self, split: str | None = None) -> dict: + """Every integer-valued entry in the recorded metrics block.""" + split = split or self.split.role + block = self.metrics.get(split) or {} + return {k: v for k, v in block.items() if isinstance(v, int)} + + def run(self, *args, **overrides): + """Run this arm with ITS TUNED PARAMETERS. There is no other way in. + + THE INCIDENT THIS PREVENTS: the original run grid-searched the baseline + on the train split and saved the winners (threshold=0.5, + min_duration_ms=10, merge_gap_ms=50). Every re-run afterwards called the + function positionally and silently got the signature defaults + (0.7 / 30 / 20), so the baseline arm was handicapped before a single + unit was matched. + + Parameters cannot be overridden here. An arm run with different + parameters is a different arm and needs its own tuning on the train + split and its own registration. + """ + if overrides: + raise IncomparableError( + f"\nREFUSING TO RUN {self.name!r} with overrides " + f"{sorted(overrides)}.\n" + f" An arm is bound to the parameters it was tuned to " + f"({dict(self.params)}). Overriding them here reproduces the " + f"untuned-parameter failure: the arm runs at values nobody " + f"tuned, and the score is then compared as if it had been.\n" + f" If you want other parameters: tune them on the TRAIN split " + f"and register_baseline() the result under a new name.\n") + if self.fn is None: + raise IncomparableError( + f"\nREFUSING TO RUN {self.name!r}: no callable registered.\n" + f" This record carries recorded metrics only (source: " + f"{self.source}), so it can be compared but not re-run.\n") + _check_params_accepted(self.fn, self.params, self.name) + return self.fn(*args, **dict(self.params)) + + def describe(self) -> str: + prov = ", ".join(f"{k}={v}" for k, v in sorted(self.provenance.items())) + metrics = ", ".join( + f"{k}: " + " ".join(f"{mk}={mv}" for mk, mv in sorted(v.items())) + for k, v in sorted(self.metrics.items())) + return "\n".join([ + f"arm {self.name!r}", + f" callable {self.fn_ref}", + f" params {dict(self.params)}", + f" split {self.split.describe()}", + f" criterion {self.criterion}", + f" source {os.path.basename(self.source)}", + f" metrics {metrics or '(none recorded)'}", + f" provenance {prov or '(none recorded)'}", + ]) + + +def _accepted_kwargs(fn: Callable) -> set[str]: + code = getattr(fn, "__code__", None) + if code is None: # builtin / C callable: cannot introspect + return set() + return set(code.co_varnames[:code.co_argcount + code.co_kwonlyargcount]) + + +def _check_params_accepted(fn: Callable, params: Mapping[str, Any], + name: str) -> None: + """Refuse to bind tuned parameters a callable will not accept. + + THE INCIDENT THIS PARTLY PREVENTS: one variant's tuned winners included two + parameters the other variant's function does not accept. The original runner + filtered unknown keys out with a `co_varnames` comprehension, so handing one + method's parameters to the other's callable silently dropped two of them and + scored something nobody had tuned. Dropping a tuned parameter is now an + error, not a filter. + + Honest limitation: this is one-directional. If the wrong callable happens to + accept every parameter name, the mismatch passes. Only + `register_baseline_from_results()` — which reads the callable and the params + from one declared method key at once — closes that. + """ + accepted = _accepted_kwargs(fn) + if not accepted: + return + unknown = sorted(set(params) - accepted) + if unknown: + raise IncomparableError( + f"\nREFUSING TO REGISTER/RUN {name!r}: tuned parameter(s) " + f"{unknown} are not accepted by " + f"{getattr(fn, '__module__', '?')}.{getattr(fn, '__name__', fn)}" + f"{tuple(sorted(accepted))}.\n" + f" The tuned parameters and the callable do not belong to the same " + f"method. Silently dropping them means the arm runs parameters " + f"nobody tuned, under a function nobody validated.\n" + f" Check which method key produced those parameters in the results " + f"file and register that method's callable.\n") + + +# --------------------------------------------------------------------------- +# Registry +# --------------------------------------------------------------------------- + +_REGISTRY: dict[str, Baseline] = {} + + +def clear_registry() -> None: + """Empty the registry. For tests and for scripts that build their own set.""" + _REGISTRY.clear() + + +def register_baseline(name: str, fn: Callable | None, params: Mapping[str, Any], + split: "Split | Sequence[str] | str", + criterion: Criterion, source: str, + metrics: Mapping | None = None, + n_units: int | None = None, + fn_ref: str | None = None, + provenance: Mapping[str, str] | None = None, + replace: bool = False) -> Baseline: + """Register an arm as callable + params + split + criterion + source. + + THE INCIDENT THIS PREVENTS: the comparator used to be the string + `"Baseline: P=0.860, R=0.636, F1=0.731"`, printed by eight scripts. It + reached a job-completion notification, was read back as a measurement, and + became "the new model still beats the baseline" — comparing IoU > 0.1 on a + 499-unit split against IoU >= 0.3 OR overlap > 50% on a 376-unit split. + There is no way to register a bare number here. + + `split` accepts a `Split`, a sequence of member ids (hashed into one), or a + bare id string. A bare id disables the "which members differ" diff, so pass + the member list when you have it. + """ + if isinstance(split, Split): + sp = split + if sp.n_units is None and n_units is not None: + sp = _dc_replace(sp, n_units=n_units) + elif isinstance(split, str): + sp = Split(split_id=split, n_units=n_units, source=source, + provenance="id-only (no member list; diffs unavailable)") + else: + sp = Split.from_items(split, n_units=n_units, source=source) + + if not isinstance(criterion, Criterion): + raise IncomparableError( + f"\nREFUSING TO REGISTER {name!r}: criterion must be a Criterion, " + f"got {type(criterion).__name__} {criterion!r}.\n" + f" A free-text criterion is the original failure — 'IoU > 0.1' and " + f"'IoU >= 0.3 OR overlap > 50%' are both truthy strings and nothing " + f"compares them. Use Criterion(...) or Criterion.from_results(...).\n") + + params = dict(params or {}) + if fn is not None: + _check_params_accepted(fn, params, name) + fn_ref = fn_ref or (f"{getattr(fn, '__module__', '?')}." + f"{getattr(fn, '__name__', repr(fn))}") + if name in _REGISTRY and not replace: + raise IncomparableError( + f"\nREFUSING TO REGISTER {name!r}: already registered from " + f"{_REGISTRY[name].source}.\n" + f" Two different records under one name is how 'the baseline' came " + f"to mean three different things. Pick a distinct name, or pass " + f"replace=True if you really mean to shadow it.\n") + + bl = Baseline(name=name, fn=fn, params=params, split=sp, + criterion=criterion, source=source, + metrics=dict(metrics or {}), fn_ref=fn_ref or "", + provenance=dict(provenance or {})) + _REGISTRY[name] = bl + return bl + + +def register_baseline_from_results( + name: str, results_path: str, criterion_keys: Sequence[str], + method: str | None = None, fn: Callable | None = None, + methods_key: str = "results_by_method", + params_key: str = "parameters", + metrics_key: str = "aggregate", + split_key: str = "test_items", + criterion_block_key: str = "matching_criteria", + role: str = "test", + n_units_keys: Sequence[str] = ("tp", "fn"), + criterion: Criterion | None = None, + replace: bool = False, + **asserted_criterion_fields: Any) -> Baseline: + """Register an arm whose params, split, criterion and metrics all come from one file. + + THE INCIDENT THIS PREVENTS (two failures at once): the headline number was + produced by one function with one tuned parameter set. A later script picked + its callable by *name* — landing on a sibling variant that scores 0.442 + where the real one scores 0.731 — and its parameters from the function + signature defaults. Here the parameters and the metrics are read out of the + same declared method key in one step, so they provably describe the same + run, and the callable is checked against those parameters. + + Everything except the callable and the asserted criterion fields is + file-derived, and `provenance` records which is which. + + WHAT THE PRODUCING SCRIPT MUST SAVE for this to work: + - `{methods_key}.{method}.{params_key}` — the tuned parameters + - `{methods_key}.{method}.{metrics_key}.{role}` — the metrics for the split + - `{split_key}` — the split's member list, not a description of it + - `{criterion_block_key}` — the fields named in `criterion_keys` + If it saves none of those, the arm cannot be registered without asserting + fields, which is the failure the registry exists to stop. + """ + with open(results_path) as fh: + data = json.load(fh) + base = os.path.basename(results_path) + + node = data + if method is not None: + by_method = data.get(methods_key) or {} + if method not in by_method: + raise IncomparableError( + f"\nREFUSING: {base} has no {methods_key}[{method!r}] " + f"(has: {sorted(by_method)}).\n") + node = by_method[method] + + params = node.get(params_key) + if params is None: + raise IncomparableError( + f"\nREFUSING TO REGISTER {name!r}: no {params_key!r} block for " + f"{method!r} in {base}.\n" + f" An arm without its tuned parameters runs at library signature " + f"defaults, and is then compared as if it had been tuned.\n") + + metrics = dict(node.get(metrics_key) or {}) + if role not in metrics: + raise IncomparableError( + f"\nREFUSING TO REGISTER {name!r}: no {role!r} block in " + f"{metrics_key!r} (has: {sorted(metrics)}).\n") + + members = data.get(split_key) + if not members: + raise IncomparableError( + f"\nREFUSING TO REGISTER {name!r}: {base} does not record " + f"{split_key!r}.\n" + f" Without the split contents the split can only be identified by " + f"a label, and two different splits both labelled 'seed 42, 50/50' " + f"is the original failure. Have the producing script save its " + f"member lists.\n") + + split = Split.from_items( + members, n_units=n_units_from_counts(metrics.get(role), n_units_keys), + role=role, source=base, provenance="file") + + crit = criterion or Criterion.from_results( + results_path, criterion_keys, block_key=criterion_block_key, + **asserted_criterion_fields) + + prefix = f"{base}:{methods_key}.{method}" if method else base + prov = { + "params_from": f"{prefix}.{params_key}", + "metrics_from": f"{prefix}.{metrics_key}", + "split_from": f"{base}:{split_key}", + "criterion_from": ( + "asserted by caller" if criterion is not None + else f"{base}:{criterion_block_key}" + + (f" (asserted: {sorted(asserted_criterion_fields)})" + if asserted_criterion_fields else "")), + "method_key": str(method), + } + if "best_method" in data: + prov["best_method_in_file"] = str(data["best_method"]) + + return register_baseline(name, fn, params, split, crit, results_path, + metrics=metrics, provenance=prov, replace=replace) + + +def get_baseline(name: str) -> Baseline: + """Look up a registered arm. Never reconstruct one by hand instead. + + THE INCIDENT THIS PREVENTS: when the comparator was not obtainable in one + call, each comparison script built its own — one re-wrote the baseline from + scratch (recall 0.636 -> 0.098), the next imported the wrong variant (0.731 + vs 0.442). One call, one record, four welded fields. + """ + if name not in _REGISTRY: + raise IncomparableError( + f"\nREFUSING: no arm registered as {name!r} " + f"(registered: {sorted(_REGISTRY)}).\n" + f" Register it with register_baseline_from_results(...) so its " + f"params, split, criterion and metrics all come from the file that " + f"produced them. Do not inline a number.\n") + return _REGISTRY[name] + + +def list_baselines() -> list[str]: + return sorted(_REGISTRY) + + +def _fmt(v: Any) -> str: + return "None" if v is None else repr(v) + + +def compare(a: "Baseline | str", b: "Baseline | str", metric: str = "f1", + split: str = "test", max_members_shown: int = 3) -> dict: + """Compute a delta, or refuse and say exactly why. THE load-bearing function. + + A delta must be *uncomputable* when the arms are not comparable, so this + raises `IncomparableError` unless `a.split_id == b.split_id` and + `a.criterion == b.criterion`, plus a separate ground-truth-count check that + catches "same members, different label extraction". + + THE INCIDENT THIS PREVENTS: "the new model beats the baseline" was computed + between an arm scored at IoU > 0.1 on a 499-unit split and an arm scored at + IoU >= 0.3 OR overlap > 50% on a 376-unit split. Every arithmetic step was + correct. The subtraction was meaningless, and nothing in the pipeline was + positioned to say so. This is that position. + + Returns both arms' metrics, the delta, the shared split id and criterion, + and `provenance_warnings` for any field that was asserted or reconstructed + by a human rather than read from a results file — because a registry can + require a field it cannot verify. + """ + a = a if isinstance(a, Baseline) else get_baseline(a) + b = b if isinstance(b, Baseline) else get_baseline(b) + + problems: list[str] = [] + + if a.split_id != b.split_id: + only_a, only_b = a.split.diff(b.split) + lines = [" SPLIT MISMATCH", + f" {a.name}: split_id {a.split.describe()}", + f" {b.name}: split_id {b.split.describe()}"] + if a.split.items and b.split.items: + lines.append(f" -> {len(only_a)} member(s) only in {a.name}, " + f"{len(only_b)} only in {b.name}") + for label, items in ((a.name, only_a), (b.name, only_b)): + for m in items[:max_members_shown]: + lines.append(f" only in {label}: {m}") + if len(items) > max_members_shown: + lines.append(f" ... and {len(items) - max_members_shown} " + f"more only in {label}") + else: + lines.append(" -> member lists not both recorded, so the " + "per-member diff is unavailable") + problems.append("\n".join(lines)) + else: + na, nb = a.split.n_units, b.split.n_units + if na is not None and nb is not None and na != nb: + problems.append( + f" GROUND-TRUTH COUNT MISMATCH on one split_id ({a.split_id})\n" + f" {a.name}: {na} GT units\n" + f" {b.name}: {nb} GT units\n" + f" -> identical split members, different ground truth. The " + f"label extraction differs between the two runs.") + + crit_diff = a.criterion.differences(b.criterion) + if crit_diff: + problems.append( + f" CRITERION MISMATCH\n" + f" {a.name}: {a.criterion}\n" + f" {b.name}: {b.criterion}\n" + f" -> differs in: " + ", ".join( + f"{f} ({_fmt(va)} vs {_fmt(vb)})" for f, va, vb in crit_diff)) + + if problems: + raise IncomparableError( + f"\nREFUSING TO COMPARE {a.name!r} against {b.name!r}: the arms are " + f"not comparable.\n\n" + "\n\n".join(problems) + "\n\n" + f" WHAT TO DO — a delta between these is not a smaller or larger " + f"number, it has no meaning:\n" + f" 1. Choose ONE split. Use its member list, not a description " + f"of how it was generated.\n" + f" 2. Choose ONE Criterion(...) and score both arms under it.\n" + f" 3. Re-score BOTH arms — the registered ones via " + f"get_baseline().run(...), which is bound to their tuned " + f"parameters.\n" + f" 4. Register the re-scored arms and compare() those.\n" + f" Do not report the delta you were about to report.\n") + + va, vb = a.metric(metric, split), b.metric(metric, split) + warnings = [f"{arm.name}.{k} = {v}" for arm in (a, b) + for k, v in sorted(arm.provenance.items()) + if str(v).startswith("asserted") or "reconstructed" in str(v)] + return { + "metric": metric, + "split": split, + "split_id": a.split_id, + "n_gt_units": a.split.n_units, + "criterion": a.criterion.as_dict(), + "a": {"name": a.name, "value": va, "counts": a.counts(split), + "source": os.path.basename(a.source), "params": dict(a.params), + "callable": a.fn_ref}, + "b": {"name": b.name, "value": vb, "counts": b.counts(split), + "source": os.path.basename(b.source), "params": dict(b.params), + "callable": b.fn_ref}, + "delta": va - vb, + "provenance_warnings": warnings, + } + + +# --------------------------------------------------------------------------- +# Guards +# --------------------------------------------------------------------------- + +def assert_sweep_interior(results: Sequence[Mapping], key: str = "f1", + param_key: str = "threshold") -> None: + """Refuse to report an optimum sitting at the edge of the swept range. + + THE INCIDENT THIS PREVENTS: two runs both selected threshold 0.3 — the + *lowest* value tested — so the true operating point was never located and + the headline number was the boundary of where the sweep stopped, not the + model's best. It was then read as "the model is confident about a lot of + boundaries", which inverts the meaning: needing a low threshold means the + metric only peaks once low-confidence predictions are accepted, which is + evidence of weak calibration, not strength. + + Raises `IncomparableError` (a `SystemExit` subclass) rather than returning a + flag, because the alternative is a number that gets quoted. + """ + if len(results) < 2: + return + values = sorted(r[param_key] for r in results) + best = max(results, key=lambda r: r[key]) + if best[param_key] in (values[0], values[-1]): + edge = "lowest" if best[param_key] == values[0] else "highest" + raise IncomparableError( + f"\nREFUSING TO REPORT: best {key} is at {param_key}=" + f"{best[param_key]}, the {edge} value swept ({values}).\n" + f" The optimum is at the edge of the range, so the real optimum " + f"was never tested.\n" + f" Extend the sweep past {best[param_key]} and re-run before " + f"quoting this number.\n") + + +# --------------------------------------------------------------------------- +# Reproducibility / provenance +# --------------------------------------------------------------------------- + +def set_all_seeds(seed: int = 42) -> dict: + """Seed every RNG that touches a result, and report what was seeded. + + THE INCIDENT THIS PREVENTS: `random.seed(seed)` alone fixed only the + file-level split. Weight init, dropout and dataloader shuffling all drew + from numpy/torch, which nothing seeded — so the same script on the same data + produced F1 0.7675 where it had produced 0.7839, and the published number + could not be regenerated. The discrepancy was then explained as "a different + random split", which was the one thing that *was* controlled. + + Call it first thing in `main()` and put the return value in the provenance + block. `numpy` and `torch` are optional — what is seeded is reported, so an + absent library cannot look like a seeded one. + """ + info: dict = {"seed": seed, "seeded": ["random"]} + random.seed(seed) + try: + import numpy as np + np.random.seed(seed) + info["seeded"].append("numpy") + info["numpy_version"] = np.__version__ + except ImportError: + pass + try: + import torch + torch.manual_seed(seed) + torch.cuda.manual_seed_all(seed) + try: + # cuDNN autotuning picks different kernels run to run. A no-op on + # CPU, but it costs nothing and makes a later GPU move reproducible. + torch.backends.cudnn.deterministic = True + torch.backends.cudnn.benchmark = False + except Exception: + pass + info["seeded"].append("torch") + info["torch_version"] = torch.__version__ + except ImportError: + pass + return info + + +def run_stamped(path: str, when: "datetime | None" = None) -> str: + """Insert a UTC run stamp before the extension. + + THE INCIDENT THIS PREVENTS: a rerun wrote to the same fixed filename as the + run whose F1 a public page cited, replacing it in place. The published + number then existed nowhere on disk. An artifact behind a published figure + must be immutable, so results go to a new path every run. + """ + when = when or datetime.now(timezone.utc) + root, ext = os.path.splitext(path) + return f"{root}.{when:%Y%m%dT%H%M%SZ}{ext}" + + +def _git_sha(cwd: str | None = None) -> str | None: + try: + out = subprocess.run(["git", "rev-parse", "--short", "HEAD"], + cwd=cwd or os.getcwd(), capture_output=True, + text=True, timeout=5) + return out.stdout.strip() or None + except Exception: + return None + + +def provenance_block(seed_info: Mapping | None = None, + cwd: str | None = None, **extra) -> dict: + """The block every results JSON must carry to be reproducible from itself. + + A result is not finished until a second script can regenerate its headline + number from the JSON alone. That needs the seed, the library versions, the + script, the git sha, and every data-reduction decision — not just the + metrics. See `docs/results-provenance-checklist.md` for the full list; this + function supplies the parts that can be collected automatically. The rest + (the split member lists, the criterion, how the operating point was chosen, + every truncation and cap) you must pass in as `**extra`, because only the + calling script knows them. + """ + p = { + "generated_utc": datetime.now(timezone.utc).isoformat(timespec="seconds"), + "script": os.path.basename(sys.argv[0]) or "", + "git_sha": _git_sha(cwd), + "python": sys.version.split()[0], + } + if seed_info: + p.update(dict(seed_info)) + p.update(extra) + return p diff --git a/tests/test_comparison.py b/tests/test_comparison.py new file mode 100644 index 0000000..b84b78c --- /dev/null +++ b/tests/test_comparison.py @@ -0,0 +1,434 @@ +"""The comparison registry must refuse the four invalidities it was built for. + +Each test names the failure it reproduces. The registry's value is entirely in +what it *refuses*, so almost every assertion here is that a call raises rather +than returns a number. See docs/trustworthy-comparison.md for the case study and +docs/baseline-registry.md for the API walkthrough. +""" +from __future__ import annotations + +import json +import os +import sys + +import pytest + +sys.path.insert(0, os.path.normpath(os.path.join(os.path.dirname(__file__), ".."))) + +from matilde_plugin.engine.comparison import ( # noqa: E402 + Baseline, + Criterion, + IncomparableError, + Split, + assert_sweep_interior, + clear_registry, + compare, + get_baseline, + list_baselines, + n_units_from_counts, + provenance_block, + register_baseline, + register_baseline_from_results, + run_stamped, + set_all_seeds, + split_id_of, +) + +STRICT = dict(iou_threshold=0.3, overlap_threshold=0.5, iou_op=">=") +LOOSE = dict(iou_threshold=0.1, overlap_threshold=None, iou_op=">") + +ITEMS_A = [f"rec_{i:03d}" for i in range(30)] +ITEMS_B = ITEMS_A[:-1] + ["rec_999"] # same size, one member swapped + + +@pytest.fixture(autouse=True) +def _clean_registry(): + clear_registry() + yield + clear_registry() + + +def _detector(data, threshold=0.7, min_duration_ms=30, merge_gap_ms=20): + """Stand-in for a tuned classical detector. Signature defaults are the + untuned values — the same trap as the real one.""" + return [threshold, min_duration_ms, merge_gap_ms, data] + + +def _register(name, items, criterion_fields, metrics=None, fn=None, + params=None, provenance=None): + return register_baseline( + name, fn, params or {}, items, Criterion(**criterion_fields), + source=f"{name}_results.json", + metrics=metrics or {"test": {"f1": 0.5, "tp": 100, "fp": 10, "fn": 20}}, + n_units=n_units_from_counts((metrics or {}).get("test") + or {"tp": 100, "fn": 20}), + provenance=provenance) + + +# --------------------------------------------------------------------------- +# Split identity — "the seed-42 50/50 split" described two different splits +# --------------------------------------------------------------------------- + +def test_split_id_is_content_derived_and_order_independent(): + assert split_id_of(ITEMS_A) == split_id_of(list(reversed(ITEMS_A))) + + +def test_one_swapped_member_is_a_different_split_at_the_same_size(): + assert len(ITEMS_A) == len(ITEMS_B) + assert split_id_of(ITEMS_A) != split_id_of(ITEMS_B) + + +def test_a_label_cannot_be_a_split_id(): + # The root cause of failure 1: the split travelled as the *description* + # "seed 42, 50/50", which two different splits both satisfied. + with pytest.raises(TypeError) as e: + split_id_of("the seed-42 50/50 split") + assert "CONTENTS" in str(e.value) + + +def test_empty_split_is_refused(): + with pytest.raises(ValueError): + split_id_of([]) + + +# --------------------------------------------------------------------------- +# Criterion — prose is not comparable; structure is +# --------------------------------------------------------------------------- + +def test_note_is_excluded_from_equality_in_both_directions(): + a = Criterion(note="IoU > 0.3 OR overlap > 50%", **STRICT) + b = Criterion(note="a completely different sentence", **STRICT) + assert a == b # rewording cannot split a criterion + assert hash(a) == hash(b) + assert Criterion(**STRICT) != Criterion(**LOOSE) + + +def test_declared_none_differs_from_absent(): + # "no overlap fallback" and "never said whether there is one" are different + # states; conflating them lets a missing fallback pass for a declared one. + declared = Criterion(iou_threshold=0.3, overlap_threshold=None) + absent = Criterion(iou_threshold=0.3) + assert declared != absent + assert ("overlap_threshold", None, "") in declared.differences(absent) + + +def test_differences_names_which_field_disagrees(): + diff = dict((f, (a, b)) for f, a, b in + Criterion(**STRICT).differences(Criterion(**LOOSE))) + assert diff["iou_threshold"] == (0.3, 0.1) + assert diff["iou_op"] == (">=", ">") + + +def test_empty_criterion_is_refused(): + with pytest.raises(ValueError): + Criterion() + + +def test_criterion_is_immutable(): + with pytest.raises(AttributeError): + Criterion(**STRICT).iou_threshold = 0.9 + + +def test_from_results_takes_numbers_from_file_and_operators_from_caller(tmp_path): + # The file's own prose said "IoU > 0.3" while the scoring code evaluated + # `>=`. Parsing the operator out of the sentence would encode that error. + p = tmp_path / "r.json" + p.write_text(json.dumps({"matching_criteria": { + "iou_threshold": 0.3, "overlap_threshold": 0.5, + "method": "IoU > 0.3 OR overlap > 50%"}})) + c = Criterion.from_results(str(p), ("iou_threshold", "overlap_threshold"), + iou_op=">=") + assert c.get("iou_threshold") == 0.3 + assert c.get("iou_op") == ">=" # asserted by the caller + assert "IoU > 0.3" in c.note # the wrong prose, quarantined + assert c == Criterion(**STRICT) + + +def test_from_results_refuses_a_file_that_records_no_criterion(tmp_path): + p = tmp_path / "r.json" + p.write_text(json.dumps({"best": {"f1": 0.78}})) + with pytest.raises(IncomparableError) as e: + Criterion.from_results(str(p), ("iou_threshold",)) + assert "asserted from memory" in str(e.value) + + +# --------------------------------------------------------------------------- +# Failure 1 — a hardcoded comparator, and the mismatches it hid +# --------------------------------------------------------------------------- + +def test_a_bare_number_cannot_be_registered(): + with pytest.raises(IncomparableError) as e: + register_baseline("x", None, {}, ITEMS_A, "IoU >= 0.3 OR overlap > 50%", + source="notification stdout") + assert "must be a Criterion" in str(e.value) + + +def test_compare_refuses_a_split_mismatch_and_names_the_members(): + _register("classical", ITEMS_A, STRICT) + _register("learned", ITEMS_B, STRICT) + with pytest.raises(IncomparableError) as e: + compare("classical", "learned") + msg = str(e.value) + assert "SPLIT MISMATCH" in msg + assert "1 member(s) only in classical" in msg + assert "rec_999" in msg # says which + + +def test_compare_refuses_a_criterion_mismatch_and_names_the_field(): + _register("classical", ITEMS_A, STRICT) + _register("learned", ITEMS_A, LOOSE) + with pytest.raises(IncomparableError) as e: + compare("classical", "learned") + msg = str(e.value) + assert "CRITERION MISMATCH" in msg + assert "iou_threshold (0.3 vs 0.1)" in msg + + +def test_compare_refuses_same_members_different_ground_truth(): + # The 499-vs-376 discrepancy surfaced here first, before anyone noticed the + # member lists differed: identical split, different label extraction. + _register("a", ITEMS_A, STRICT, + metrics={"test": {"f1": 0.73, "tp": 239, "fp": 39, "fn": 137}}) + _register("b", ITEMS_A, STRICT, + metrics={"test": {"f1": 0.77, "tp": 449, "fp": 224, "fn": 50}}) + with pytest.raises(IncomparableError) as e: + compare("a", "b") + msg = str(e.value) + assert "GROUND-TRUTH COUNT MISMATCH" in msg + assert "376 GT units" in msg and "499 GT units" in msg + + +def test_the_refusal_says_what_to_do(): + _register("a", ITEMS_A, STRICT) + _register("b", ITEMS_B, LOOSE) + msg = str(pytest.raises(IncomparableError, compare, "a", "b").value) + assert "Choose ONE split" in msg + assert "Do not report the delta you were about to report" in msg + + +def test_a_matched_pair_compares_and_carries_its_provenance(): + _register("classical", ITEMS_A, STRICT, + metrics={"test": {"f1": 0.801, "tp": 363, "fp": 90, "fn": 136}}) + _register("learned", ITEMS_A, STRICT, + metrics={"test": {"f1": 0.775, "tp": 447, "fp": 130, "fn": 52}}, + provenance={"criterion_from": "asserted: train.py:206"}) + res = compare("classical", "learned") + assert res["delta"] == pytest.approx(0.026, abs=1e-9) + assert res["split_id"] == split_id_of(ITEMS_A) + # A field a human asserted is surfaced, not hidden: the registry can require + # a criterion and still not verify one its source file never recorded. + assert res["provenance_warnings"] == ["learned.criterion_from = asserted: train.py:206"] + + +def test_an_id_only_split_says_the_diff_is_unavailable(): + register_baseline("a", None, {}, "deadbeefcafe", Criterion(**STRICT), "a.json", + metrics={"test": {"f1": 0.5}}) + register_baseline("b", None, {}, ITEMS_A, Criterion(**STRICT), "b.json", + metrics={"test": {"f1": 0.6}}) + msg = str(pytest.raises(IncomparableError, compare, "a", "b").value) + assert "per-member diff is unavailable" in msg + + +def test_train_metrics_are_not_silently_substituted_for_test(): + _register("a", ITEMS_A, STRICT, metrics={"train": {"f1": 0.83}}) + _register("b", ITEMS_A, STRICT, metrics={"train": {"f1": 0.80}}) + msg = str(pytest.raises(IncomparableError, compare, "a", "b").value) + assert "no 'test' metrics recorded" in msg + assert "how a train-split score gets quoted as test" in msg + + +def test_two_records_cannot_share_one_name(): + # "the baseline" came to mean three different things this way. + _register("baseline", ITEMS_A, STRICT) + with pytest.raises(IncomparableError) as e: + _register("baseline", ITEMS_B, LOOSE) + assert "already registered" in str(e.value) + assert list_baselines() == ["baseline"] + + +def test_an_unregistered_arm_is_refused_not_invented(): + msg = str(pytest.raises(IncomparableError, get_baseline, "baseline").value) + assert "Do not inline a number" in msg + + +# --------------------------------------------------------------------------- +# Failures 2 and 3 — reimplementation, and the wrong function +# --------------------------------------------------------------------------- + +def test_the_registered_callable_is_the_validated_one(): + bl = _register("classical", ITEMS_A, STRICT, fn=_detector, + params={"threshold": 0.5}) + assert bl.fn is _detector + assert bl.fn_ref.endswith("_detector") + + +def test_params_from_one_method_cannot_be_bound_to_another_callable(): + # The other variant's tuned winners carry parameters this callable does not + # accept. The original runner filtered them out with a co_varnames + # comprehension, so the mismatch was silent and the arm ran untuned values. + with pytest.raises(IncomparableError) as e: + _register("wrong", ITEMS_A, STRICT, fn=_detector, + params={"threshold": 0.1, "n_fft": 256, "hop": 128}) + msg = str(e.value) + assert "['hop', 'n_fft']" in msg + assert "do not belong to the same" in msg + + +def test_from_results_reads_params_metrics_split_and_criterion_from_one_file(tmp_path): + p = tmp_path / "validation.json" + p.write_text(json.dumps({ + "best_method": "simple", + "test_items": ITEMS_A, + "matching_criteria": {"iou_threshold": 0.3, "overlap_threshold": 0.5, + "method": "IoU > 0.3 OR overlap > 50%"}, + "results_by_method": { + "simple": { + "parameters": {"threshold": 0.5, "min_duration_ms": 10, + "merge_gap_ms": 50}, + "aggregate": {"test": {"f1": 0.7309, "tp": 239, "fp": 39, "fn": 137}, + "train": {"f1": 0.8265, "tp": 362, "fp": 61, "fn": 91}}}}})) + bl = register_baseline_from_results( + "simple", str(p), ("iou_threshold", "overlap_threshold"), + method="simple", fn=_detector, iou_op=">=") + + # Nothing was typed in: params, metrics, split and thresholds are file-derived. + assert bl.params == {"threshold": 0.5, "min_duration_ms": 10, "merge_gap_ms": 50} + assert bl.metric("f1") == 0.7309 + assert bl.split.n_units == 376 # derived tp+fn, not typed + assert bl.split_id == split_id_of(ITEMS_A) + assert bl.criterion == Criterion(**STRICT) + assert bl.provenance["best_method_in_file"] == "simple" + assert "matching_criteria" in bl.provenance["criterion_from"] + + +def test_from_results_refuses_a_file_with_no_member_list(tmp_path): + p = tmp_path / "r.json" + p.write_text(json.dumps({ + "matching_criteria": {"iou_threshold": 0.3}, + "results_by_method": {"m": {"parameters": {}, + "aggregate": {"test": {"f1": 0.7}}}}})) + with pytest.raises(IncomparableError) as e: + register_baseline_from_results("m", str(p), ("iou_threshold",), method="m") + assert "identified by a label" in str(e.value) + + +def test_from_results_refuses_a_method_without_tuned_parameters(tmp_path): + p = tmp_path / "r.json" + p.write_text(json.dumps({ + "test_items": ITEMS_A, + "matching_criteria": {"iou_threshold": 0.3}, + "results_by_method": {"m": {"aggregate": {"test": {"f1": 0.7}}}}})) + with pytest.raises(IncomparableError) as e: + register_baseline_from_results("m", str(p), ("iou_threshold",), method="m") + assert "library signature defaults" in str(e.value) + + +def test_from_results_names_the_methods_it_does_have(tmp_path): + p = tmp_path / "r.json" + p.write_text(json.dumps({"results_by_method": {"simple": {}, "windowed": {}}})) + with pytest.raises(IncomparableError) as e: + register_baseline_from_results("x", str(p), ("iou_threshold",), method="typo") + assert "['simple', 'windowed']" in str(e.value) + + +# --------------------------------------------------------------------------- +# Failure 4 — untuned parameters +# --------------------------------------------------------------------------- + +def test_run_uses_the_tuned_parameters_not_the_signature_defaults(): + bl = _register("classical", ITEMS_A, STRICT, fn=_detector, + params={"threshold": 0.5, "min_duration_ms": 10, + "merge_gap_ms": 50}) + assert bl.run("data") == [0.5, 10, 50, "data"] # not 0.7 / 30 / 20 + + +def test_run_refuses_parameter_overrides(): + bl = _register("classical", ITEMS_A, STRICT, fn=_detector, + params={"threshold": 0.5}) + with pytest.raises(IncomparableError) as e: + bl.run("data", threshold=0.7) + assert "bound to the parameters it was tuned to" in str(e.value) + assert "tune them on the TRAIN split" in str(e.value) + + +def test_an_arm_with_no_callable_can_be_compared_but_not_re_run(): + bl = _register("learned", ITEMS_A, STRICT) + with pytest.raises(IncomparableError) as e: + bl.run("data") + assert "no callable registered" in str(e.value) + + +# --------------------------------------------------------------------------- +# Sweep boundary +# --------------------------------------------------------------------------- + +def test_an_optimum_at_the_bottom_of_the_sweep_is_refused(): + sweep = [{"threshold": t, "f1": f} for t, f in + [(0.3, 0.784), (0.5, 0.72), (0.7, 0.61)]] + with pytest.raises(IncomparableError) as e: + assert_sweep_interior(sweep) + assert "the lowest value swept" in str(e.value) + assert "Extend the sweep" in str(e.value) + + +def test_an_optimum_at_the_top_of_the_sweep_is_refused(): + sweep = [{"threshold": t, "f1": f} for t, f in + [(0.3, 0.61), (0.5, 0.72), (0.7, 0.784)]] + assert "the highest value swept" in str( + pytest.raises(IncomparableError, assert_sweep_interior, sweep).value) + + +def test_an_interior_optimum_passes(): + assert_sweep_interior([{"threshold": t, "f1": f} for t, f in + [(0.1, 0.53), (0.4, 0.80), (0.6, 0.79)]]) + + +def test_a_single_point_sweep_is_not_treated_as_an_optimum(): + assert_sweep_interior([{"threshold": 0.5, "f1": 0.9}]) + + +# --------------------------------------------------------------------------- +# Provenance helpers +# --------------------------------------------------------------------------- + +def test_set_all_seeds_reports_only_what_it_actually_seeded(): + info = set_all_seeds(7) + assert info["seed"] == 7 + assert "random" in info["seeded"] + # An absent optional library must not look like a seeded one. + for lib in ("numpy", "torch"): + assert (lib in info["seeded"]) == (f"{lib}_version" in info) + + +def test_run_stamped_never_returns_the_input_path(): + out = run_stamped("/tmp/results.json") + assert out != "/tmp/results.json" + assert out.startswith("/tmp/results.") and out.endswith(".json") + assert "Z.json" in out + + +def test_provenance_block_carries_the_automatic_fields_and_passes_extras(): + p = provenance_block(set_all_seeds(42), matching_criterion="IoU >= 0.3", + duration_cap_s=180) + for k in ("generated_utc", "script", "python", "seed", "seeded"): + assert k in p + assert p["duration_cap_s"] == 180 # only the caller knows this one + assert p["matching_criterion"] == "IoU >= 0.3" + + +# --------------------------------------------------------------------------- +# describe() is the human-readable audit surface +# --------------------------------------------------------------------------- + +def test_describe_shows_all_four_welded_fields(): + bl = _register("classical", ITEMS_A, STRICT, fn=_detector, + params={"threshold": 0.5}) + text = bl.describe() + for expected in ("callable", "params", "split", "criterion", "source", + "metrics", "provenance"): + assert expected in text + assert "threshold=0.5" in text.replace("'threshold': 0.5", "threshold=0.5") + + +def test_split_describe_admits_when_the_member_list_is_missing(): + assert "member list unavailable" in Split(split_id="abc").describe() From 4f5569fcdcdeac38d83d3e33c071b32fa3f12cfb Mon Sep 17 00:00:00 2001 From: Juniper Bevensee Date: Tue, 28 Jul 2026 15:10:34 +1200 Subject: [PATCH 2/3] =?UTF-8?q?fix(docs,comparison):=20the=20finding=20rev?= =?UTF-8?q?ersed=20=E2=80=94=20and=20the=20caveat=20was=20a=20false=20infe?= =?UTF-8?q?rence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent audit blocked this PR: the docs asserted a settled empirical conclusion that a better-controlled run contradicts. A sixth run on the model's TRUE held-out 15 files, zero leakage, thresholds chosen on validation, reverses the direction on both criteria: CNN 0.859 vs 0.721 (validated matcher) and 0.847 vs 0.630 (IoU-only). The clean margin is five times the leaky one, pointing the other way. Worse, the caveat contained a falsified inference. It argued that leakage inflated the CNN, therefore removing it made the classical detector's win stronger. Empirically the CNN scored HIGHER on the clean split (0.859) than the leaked one (0.775) — leakage was not inflating it. That reasoning was asserted because it sounded conservative and was never checked against a measurement, which is exactly what this document's own rule 14 warns against. Rewritten to state what the evidence carries: the comparison was invalid four ways, a fifth attempt favoured the classical detector, a sixth reversed it, and the margin is not stable across splits. A result that flips when you fix the protocol was never a result. Also from the audit: - comparison.py: differences() used the literal string as its missing-field marker, so a field whose VALUE was that same string was indistinguishable from an absent one. differences() returned [] for unequal criteria, and compare(), which gated on differences(), computed a delta between incomparable arms. The marker is now a unique object, compare() gates on inequality itself, and an unlocalised inequality still refuses. Regression test included; it fails against the old marker. - test_declared_none_differs_from_absent asserted the marker's literal value, coupling the test to the very detail that was broken. Now asserts semantics. - rule 12's exemplar no longer cites the reversed finding. - "eight scripts" clarified: seven printed the same non-interpolated f-string, an eighth fed a different constant into a delta. - the 0.457-0.766 spread now notes it mixes two variables. 184 passed, 7 skipped (was 183/7). Co-Authored-By: Claude Opus 5 --- docs/trustworthy-comparison.md | 75 ++++++++++++++++++++--------- matilde_plugin/engine/comparison.py | 34 +++++++++++-- tests/test_comparison.py | 27 ++++++++++- 3 files changed, 109 insertions(+), 27 deletions(-) diff --git a/docs/trustworthy-comparison.md b/docs/trustworthy-comparison.md index 7b8f0b6..b97c5c6 100644 --- a/docs/trustworthy-comparison.md +++ b/docs/trustworthy-comparison.md @@ -38,7 +38,9 @@ print(f"Baseline: P=0.860, R=0.636, F1=0.731") ``` An **f-string with no interpolation**. It printed identically on every run -regardless of data, parameters, or split. It sat in eight scripts. It went to +regardless of data, parameters, or split. Eight scripts carried a hardcoded +comparator: seven printed that same non-interpolated string, and an eighth fed a +different constant into a subtraction to produce a delta. It went to stdout, stdout went into a job-completion notification, and the notification was read back as a measurement — producing the sentence "the CNN still beats the baseline" in a single turn with zero files opened. @@ -87,32 +89,58 @@ the original runner filtered unrecognised keyword arguments out with a `co_varnames` comprehension, passing one variant's tuned parameters to the other's function *dropped two of them without a word*. -### What the valid comparison actually said +### What the valid comparison said — and then said again, differently -Re-scored properly — one split, one criterion, both arms — the classical -detector wins: +The fifth attempt fixed the protocol: one split, one criterion, both arms scored +by the same function. On that split the classical detector led: | Matching criterion | Classical detector | CNN | Δ | |---|---|---|---| | Validated matcher (IoU ≥ 0.3 **or** overlap ≥ 0.5) | **0.801** @ threshold 0.4 | 0.775 @ threshold 0.2 | −0.026 | | IoU ≥ 0.3 only | **0.762** | 0.747 | −0.014 | -**State these caveats wherever you state those numbers:** +Two caveats were attached to it, both correct: -- **Both arms are oracle-best** — each arm's operating point was chosen by - maximising F1 *on the test set*. These are upper bounds, not held-out - estimates. (See rule 4.) +- **Both arms were oracle-best** — each operating point was chosen by maximising + F1 *on the test set*. Upper bounds, not held-out estimates. (See rule 4.) - **22 of the 30 test files had leaked** into the CNN's training or validation - set. Only 8 were genuinely held out for that arm. The CNN's number is - therefore inflated — which makes the classical detector's win *stronger* - evidence, not weaker, but it is not a clean measurement of either arm. -- The margins (0.026 and 0.014) are small at this sample size. They are not - claimed as significant; they are stated as *the classical baseline was never - beaten*, which is the honest form of the finding. + set. Only 8 were genuinely held out for that arm. -The publishable result is a negative one: **the classical detector still beats -the CNN.** That is a real finding, and it took four invalid comparisons to -reach it. +A sixth run then fixed both caveats: the model's true held-out 15 files, zero +leakage, each arm's threshold chosen on a *validation* split. **The direction +reversed, on both criteria:** + +| Matching criterion | Classical detector | CNN | Δ | +|---|---|---|---| +| Validated matcher | 0.721 @ 0.7 | **0.859** @ 0.2 | **+0.138** | +| IoU ≥ 0.3 only | 0.630 | **0.847** | **+0.218** | + +The clean margin is five times the size of the leaky one, pointing the other way. + +**And the caveat contained a false inference — which is the most instructive part +of this whole episode.** The leakage note originally read: the CNN's number is +inflated by leakage, *therefore* removing the leakage makes the classical +detector's win stronger evidence. That is a plausible confound story, and it was +wrong. Empirically the CNN scored **higher** on the clean split (0.859) than on +the leaked one (0.775). Leakage was not inflating it. The reasoning was never +checked against a measurement — it was asserted because it sounded like the +conservative direction, which is exactly what **rule 14** warns against, written +three pages further down the same document. + +So the honest finding is not "the classical detector wins." It is: + +> On this dataset the comparison was invalid four different ways, a fifth +> attempt fixed the protocol and favoured the classical detector, and a sixth +> attempt fixed the remaining confounds and reversed the result. The margin is +> not stable across splits, and the test sets are small (246–499 annotated units). + +If you need a number to quote, quote the sixth: CNN ≈ 0.86 against ≈ 0.72, with +the threshold chosen on validation and no leakage, on 15 files / 246 units — and +say that a differently-drawn split gave the opposite sign. + +That is a weaker claim than either single run appears to support, and it is the +only one the evidence carries. **A result that flips when you fix the protocol was +never a result; it was an artefact with a plausible story attached.** --- @@ -168,7 +196,9 @@ score threshold of 0.10: A **2.1× difference in true positives**, from a constant that did nothing — holding the split, the matcher and the score threshold fixed, so the resampling is the only difference. Across the scripts that reported on this model, its F1 -appeared as low as 0.457 and as high as 0.766, and every one of those numbers was +appeared as low as 0.457 and as high as 0.766 — a spread that mixes two +variables (resampled vs native input, and different confidence thresholds), which +is itself the point: nothing recorded which was which. Every one of those numbers was correctly computed. > **Rule:** a constant that is declared and never read is not documentation, it @@ -302,10 +332,11 @@ like* a result. Given time, the output is quoted. seed-label agreement of 35.8%, hedges it correctly in its own print statement, and then reports a 91% purity figure elsewhere has chosen the flattering number. That choice is the misconduct, not the low number. -12. **Report negative results.** "The classical baseline still beats our model" - is a finding. A results file recording zero predictions and F1 = 0.0, and a - docstring explaining why an approach was abandoned, are among the most - valuable artifacts a project produces. +12. **Report negative results.** A results file recording zero predictions and + F1 = 0.0, and a docstring explaining why an approach was abandoned, are among + the most valuable artifacts a project produces. "Our comparison reversed when + we fixed the split" is also a finding — and a more useful one than either + direction it pointed in. 13. **Validate the comparison before you calibrate the number.** This is the subtle one. A report can be *excellent in form* — recommending a range over a point estimate, flagging that 1.7 points at n=30 is within normal diff --git a/matilde_plugin/engine/comparison.py b/matilde_plugin/engine/comparison.py index ec738cd..268e5b1 100644 --- a/matilde_plugin/engine/comparison.py +++ b/matilde_plugin/engine/comparison.py @@ -115,6 +115,11 @@ def n_units_from_counts(block: Mapping | None, # Criterion # --------------------------------------------------------------------------- +# Unique missing-field marker. Deliberately not a string: any string could +# collide with a real field value (see Criterion.differences). +_ABSENT = object() + + class Criterion: """A scoring criterion as comparable data, not prose. @@ -187,8 +192,14 @@ def differences(self, other: "Criterion") -> list[tuple[str, Any, Any]]: mine, theirs = self.fields, other.fields out = [] for k in sorted(set(mine) | set(theirs)): - a = mine.get(k, "") if k in mine else "" - b = theirs.get(k, "") if k in theirs else "" + # _ABSENT is a unique object, not the string "". A field whose + # VALUE was the literal "" used to be indistinguishable from an + # absent field, so differences() returned [] for two criteria that were + # not equal — and compare(), which gated on differences(), let the delta + # through. Contrived input, exactly the bug class this module exists to + # refuse. Found in review. + a = mine.get(k, _ABSENT) + b = theirs.get(k, _ABSENT) if a != b: out.append((k, a, b)) return out @@ -468,7 +479,8 @@ def register_baseline(name: str, fn: Callable | None, params: Mapping[str, Any], """Register an arm as callable + params + split + criterion + source. THE INCIDENT THIS PREVENTS: the comparator used to be the string - `"Baseline: P=0.860, R=0.636, F1=0.731"`, printed by eight scripts. It + `"Baseline: P=0.860, R=0.636, F1=0.731"`, printed by seven scripts (an eighth + fed a different hardcoded constant into a delta). It reached a job-completion notification, was read back as a measurement, and became "the new model still beats the baseline" — comparing IoU > 0.1 on a 499-unit split against IoU >= 0.3 OR overlap > 50% on a 376-unit split. @@ -638,6 +650,8 @@ def list_baselines() -> list[str]: def _fmt(v: Any) -> str: + if v is _ABSENT: + return "" return "None" if v is None else repr(v) @@ -694,8 +708,20 @@ def compare(a: "Baseline | str", b: "Baseline | str", metric: str = "f1", f" -> identical split members, different ground truth. The " f"label extraction differs between the two runs.") + # Gate on inequality itself, then use differences() only to explain it. The + # reverse — gating on differences() — makes the refusal only as reliable as the + # diff routine, and the docstring promises refusal whenever the criteria are + # unequal. Belt and braces: if they are unequal but the diff comes back empty, + # still refuse, and say the diff could not localise it. crit_diff = a.criterion.differences(b.criterion) - if crit_diff: + if a.criterion != b.criterion and not crit_diff: + problems.append( + f" CRITERION MISMATCH (unlocalised)\n" + f" {a.name}: {a.criterion}\n" + f" {b.name}: {b.criterion}\n" + f" -> the two criteria are not equal, but no differing field could " + f"be identified. Treat as incomparable and inspect them by hand.") + elif crit_diff: problems.append( f" CRITERION MISMATCH\n" f" {a.name}: {a.criterion}\n" diff --git a/tests/test_comparison.py b/tests/test_comparison.py index b84b78c..7510d62 100644 --- a/tests/test_comparison.py +++ b/tests/test_comparison.py @@ -109,7 +109,32 @@ def test_declared_none_differs_from_absent(): declared = Criterion(iou_threshold=0.3, overlap_threshold=None) absent = Criterion(iou_threshold=0.3) assert declared != absent - assert ("overlap_threshold", None, "") in declared.differences(absent) + # Assert the SEMANTICS, not the sentinel's value. This previously asserted the + # literal string "", which coupled the test to an implementation detail + # — and that detail was the bug: a field whose value was the string "" + # was indistinguishable from an absent field, so differences() could return [] + # for unequal criteria. The marker is now a unique object. + diff = dict((f, (a, b)) for f, a, b in declared.differences(absent)) + assert "overlap_threshold" in diff + mine, theirs = diff["overlap_threshold"] + assert mine is None # declared, explicitly no fallback + assert theirs is not None # absent — some marker, not None + assert not isinstance(theirs, (int, float, str, bool)) + + +def test_absent_marker_cannot_collide_with_a_real_value(): + """A field whose value is the string "" must still count as a difference. + + Regression: `differences()` used the literal string "" as its + missing-field marker, so this pair compared unequal via `__eq__` while + `differences()` returned [] — and `compare()`, which gated on `differences()`, + computed a delta between two incomparable arms. Found in review. + """ + tricky = Criterion(iou_threshold=0.3, extra="") + plain = Criterion(iou_threshold=0.3) + assert tricky != plain + assert tricky.differences(plain), ( + "a field valued '' must not be mistaken for an absent field") def test_differences_names_which_field_disagrees(): From 2f7834b1ff3b92ee7608f9ea3fa1a85b621336ea Mon Sep 17 00:00:00 2001 From: Juniper Bevensee Date: Tue, 28 Jul 2026 15:12:47 +1200 Subject: [PATCH 3/3] docs(readme): index rows describe method, not a named in-progress study The sanitization gate's semantic layer flagged two docs-index rows for referencing a specific engagement's particulars rather than generic methodology: the attempt count of one case study, and 'the bounded-sample validation study' as a definite thing. The docs themselves passed; only the index prose was flagged. Reworded to name the failure modes and the mechanism instead. Better writing regardless -- an index should say what a reader will learn, not how many times one team got it wrong. Note the gate is LLM-backed and non-deterministic: this same content passed on an earlier run of the branch. Worth knowing before treating one pass as clearance. Co-Authored-By: Claude Opus 5 --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 35d4e31..52f4cee 100644 --- a/README.md +++ b/README.md @@ -164,11 +164,11 @@ something specific went wrong, and says what. | Doc | What it covers | |---|---| -| [trustworthy-comparison.md](docs/trustworthy-comparison.md) | How to establish that two measured numbers are comparable before you subtract them. Case study: one comparison that was invalid four different ways across three attempts, each fix moving the bug. Plus dead constants, label columns that aren't labels, boundary optima, unseeded runs, and silent failure. | +| [trustworthy-comparison.md](docs/trustworthy-comparison.md) | How to establish that two measured numbers are comparable before you subtract them: comparator provenance, protocol matching, split identity, tuning leakage, dead constants, label columns that aren't labels, boundary optima, unseeded runs, and silent failure. Worked through a comparison whose direction reversed once the protocol was fixed. | | [baseline-registry.md](docs/baseline-registry.md) | The comparator-as-record pattern — callable + tuned params + split + criterion — and why a registry returning only a *number* prevents one of those four failures and none of the others. Includes an honest list of what it still does not catch. | | [results-provenance-checklist.md](docs/results-provenance-checklist.md) | What a results file must carry to be reproducible from itself: seed, library versions, git SHA, the split member lists, every data-reduction decision, the matching criterion, and how the operating point was chosen. | | [golden-validation-recipe.md](docs/golden-validation-recipe.md) | The offline, dependency-free worked validation — the reference shape of a correct finding, and the package's smoke test. | -| [meg-validation-study.md](docs/meg-validation-study.md) · [stateful-study-pipeline.md](docs/stateful-study-pipeline.md) | The bounded-sample validation study and the resumable step pipeline behind it. | +| [meg-validation-study.md](docs/meg-validation-study.md) · [stateful-study-pipeline.md](docs/stateful-study-pipeline.md) | Running a memory-bounded study over an open dataset, and the resumable step store that makes it restartable. | | [privacy-and-visibility.md](docs/privacy-and-visibility.md) · [promotion-and-upstream.md](docs/promotion-and-upstream.md) | The privacy model, the sanitization gate, and how a technique gets promoted out of a private overlay into this package. | | [onboarding.md](docs/onboarding.md) | Start here if you are new to the package. |