P3-02: the disjoint cluster-respecting calibration split (ADR-0004 A7) - #94
Merged
Merged
Conversation
… (P3-02)
ADR-0005 D11 pins the recalibration stack as train -> temperature-scale on a
disjoint calibration split -> prior-shift, but ADR-0004's fold vocabulary had no
calibration fold. A11 (P2-13) recorded the absence, fitted its non-gated Stage-1 T
on a seeded half of the selection_val rung as a stopgap, and named P3-02 as the
step that carves the real one. This lands ADR-0004 A7 (signed 2026-08-01) and it.
Inputs: data/processed/splits/split_assignments.parquet (git-LFS, 25,913 rows,
sha256 6388ba19...), data/processed/stage2_dataset.parquet (DVC,
digest 03263354...)
Outputs: the same split table + a `calib` column (25,913 x 21,
sha256 d51252ee..., extra.table_schema_version 1.1) and the regenerated
Stage-2 dataset (DVC md5 23f412dc..., digest c5a2c251..., 30,542 x 35)
The carve. `calib` is drawn from source == corpus AND nested_train AND
fold_random == "train" AND cluster not in selection_val (5,034 records / 2,526
clusters), by a genus-stratified whole-cluster seeded draw (CALIB_CARVE_SEED
20260801, fraction 0.10) -> 859 records / 431 clusters / 23 orders / 13 phyla
(realised 17.1%). The fold_random == "train" conjunct makes disjointness from the
graded split structural, since fold_random is whole-cluster assigned, and removes
ADR-0004 A6's gate4_eval population in full. Genus stratification is measured, not
assumed: the pool is 98.5% Firmicutes and the uniform selection_val-style draw
reaches only 6 of 25 orders where stratification reaches 23.
The negative side (the decision P3-01 flagged). The committed table has no
negative rows, so a positives-only calib cannot calibrate a binary head.
Parentless decoys are admitted from the train portion only, at
DECOY_CALIB_RATE = 0.0469, by a keyed hash under a distinct ":calib:" domain
prefix -- a second draw, not a 4-way widening of decoy_fold, which would move
decoys across the train/val/test boundary. nested_train stays null for them:
A7 says what the calibration fit may see and resolves nothing about P3-03.
Also fixes a fork this step exposed: window_dataset carried a hand-typed copy of
FOLD_SCHEME_COLUMNS that went one column stale with nothing failing. It is now
derived as splits.FOLD_SCHEME_COLUMNS - STAGE2_ONLY_FOLD_COLUMNS, content and
order unchanged, with the omission named and asserted.
Validation: PASS. Full-corpus no-leakage green, 57 passed / 0 failed (was 49).
calib intersect {val,test} = 0, selection_val = 0, gate4_eval = 0, designated LOO
holdout = 0, clade-crossing = 0, straddling clusters = 0; the 22 cluster-mates
left outside calib are all nested_role == "dropped" and 0 are nested_train.
The CI clause re-derives the carve and asserts set identity rather than reading
the boolean back; seven targeted sabotages on the real table each bite the
intended clause, and loosening_admits_the_refused is non-vacuous at baseline
(pool 5,034 -> 6,366 without the conjunct). ruff + black (CI-pinned) clean,
snakemake --lint and -n clean; both artifacts regenerated after the reformat and
reproduce byte-identical digests.
Reported, not tuned: realised calib prevalence is 0.7888 vs the in-distribution
test split's 0.7727 (+1.6 pp) -- the keyed hash drew 168 parentless decoys where
the rate's expectation is 188.9. The rate is the pinned quantity; the prevalence
is a measurement (CLAUDE.md 10.3). Inherit-only would sit 15.4 pp off.
Claude-Session: https://claude.ai/code/session_01Tr67opmzHJAGBJPAb2aTdX
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesCalibration split support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SplitBuilder
participant CalibrationCarve
participant Stage2Dataset
participant LeakageTests
SplitBuilder->>CalibrationCarve: build calibration assignments
CalibrationCarve-->>SplitBuilder: return calib column and provenance
SplitBuilder->>Stage2Dataset: provide committed split fields
Stage2Dataset->>Stage2Dataset: assign parentless decoy calib values
Stage2Dataset->>LeakageTests: validate calibration invariants
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Implements ADR-0004 A7 by introducing a disjoint, cluster-respecting calib fold carved from within the nested training fold, and wires Stage 2 to consume (not recompute) this calibration membership including a deterministic parentless-decoy calibration draw.
Changes:
- Add
calibas a committed split-table column (schema v1.1) with a re-derivable, genus-stratified whole-cluster carve and disjointness assertions. - Extend Stage-2 dataset construction to carry and validate
calib, including keyed-hash admission of a fraction of train-fold parentless decoys. - Update Stage-1 fold column handling to derive from
splits.FOLD_SCHEME_COLUMNSwith an explicit Stage-2-only omission, plus expanded unit/ML/golden tests and regenerated artifacts.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_window_dataset.py | Adds a relationship-based test pinning Stage-1 fold columns as splits minus named Stage-2-only columns. |
| tests/unit/test_stage2_dataset.py | Adds fixture coverage and unit tests for deterministic, train-only decoy calibration draw behavior. |
| tests/unit/test_split_table_schema.py | Expands schema fixtures and adds unit tests for the calibration carve properties and inheritance. |
| tests/ml/test_no_leakage.py | Adds no-leakage clauses that re-derive and identity-check the committed calib carve on real data. |
| tests/golden/test_stage2_dataset_golden.py | Extends the golden gate to assert calib presence, two-class non-degeneracy, and disjointness. |
| tests/fixtures/stage2_dataset/expected.sha256 | Updates expected digest for regenerated Stage-2 dataset fixture. |
| src/tbox_finder/stage2/dataset.py | Introduces DECOY_CALIB_RATE, decoy_calib, calibration reporting, and build-time invariants for calib. |
| src/tbox_finder/splits.py | Adds calibration carve constants, eligibility and carve algorithms, schema bump to 1.1, and disjointness assertions. |
| src/tbox_finder/data/window_dataset.py | Replaces hand-copied Stage-1 FOLD_SCHEME_COLUMNS with a derived tuple excluding Stage-2-only columns. |
| docs/decisions/ADR-0004-split-and-leakage-policy.md | Documents and records acceptance of Amendment A7 and its pinned carve/validation clauses. |
| data/processed/stage2_dataset.provenance.json | Regenerates provenance metadata for the Stage-2 dataset artifact. |
| data/processed/stage2_dataset.parquet.dvc | Updates DVC pointer (md5/size) for regenerated Stage-2 parquet. |
| data/processed/splits/split_assignments.provenance.json | Adds calib provenance block and records table schema version 1.1. |
| data/processed/splits/split_assignments.parquet | Updates Git LFS pointer for the regenerated split assignments parquet. |
| data/processed/audits/stage2_dataset_report.json | Adds calib report block (counts, prevalence, disjointness checks). |
| analyses/phase3_log.qmd | Logs the P3-02 step, rationale, measured outcomes, and validation results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+172
to
+183
| def test_calib_carve_is_deterministic_and_seed_sensitive(): | ||
| """Same seed ⇒ same clusters; a different seed ⇒ different ones. | ||
|
|
||
| The second half is what makes the CI identity clause meaningful: if the draw were | ||
| seed-insensitive, re-deriving it would agree with *any* committed column and the | ||
| check would be a tautology. | ||
| """ | ||
| pytest.importorskip("pandas") | ||
| cols = _carve_cols(splits.build_split_table(_interim_frame())) | ||
| assert splits.calib_cluster_ids(**cols) == splits.calib_cluster_ids(**cols) | ||
| other = splits.calib_cluster_ids(**cols, seed=splits.CALIB_CARVE_SEED + 1) | ||
| assert isinstance(other, frozenset) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/tbox_finder/stage2/dataset.py (1)
631-636: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider naming the split-table schema requirement explicitly.
A parented decoy and a positive both take
calibfrom the joined split row through_carried. A split table written beforeCOMMITTED_TABLE_SCHEMA_VERSION = "1.1"has nocalibcolumn. The build then fails either with aKeyErrorinside_carriedor with the generic messagecalib is null on at least one row (fold/provenance gate). Neither names the cause.Add an early check in
build_datasetthat thesplitsframe carriescalib, and raise a message that states the required split-table schema version.♻️ Proposed early check
corpus = corpus.reset_index(drop=True) record_ids = ingest.compute_record_hashes(corpus) + + # ADR-0004 A7: `calib` is carried, never recomputed here, so a pre-1.1 split table + # cannot supply it. Refuse by name rather than surfacing a KeyError or a null. + if "calib" not in splits.columns: + raise ValueError( + "the split table has no 'calib' column; Stage-2 requires committed table " + "schema 1.1 (ADR-0004 A7) — regenerate the split-assignment table" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tbox_finder/stage2/dataset.py` around lines 631 - 636, In build_dataset, add an early validation that the splits DataFrame contains the calib column before downstream _carried processing. If it is absent, raise an explicit error stating that split-table schema version 1.1 or later is required, while preserving existing behavior when the column is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tbox_finder/splits.py`:
- Around line 1460-1465: Update the non_corpus validation in
src/tbox_finder/splits.py lines 1460-1465 to exempt sources in DERIVED_SOURCES,
so only external rows with calib raise. Update tests/ml/test_no_leakage.py lines
156-163 to permit derived sources in calib_outside_its_pool and assert that each
derived calib row inherits and matches its parent’s calib value.
- Around line 1334-1344: Update the stratum assignment around stratum_of so each
eligible cluster collects all non-missing resolved_genus values and assigns a
deterministic representative, such as the minimum name, instead of retaining the
first encountered value. Preserve CALIB_UNASSIGNED_STRATUM for clusters without
any non-missing genus, and add a test covering a heterogeneous cluster whose
calibration result remains unchanged when eligible rows are reordered.
In `@src/tbox_finder/stage2/dataset.py`:
- Around line 762-771: Thread decoy_fold_seed through the calibration invariant
checks so the parentless decoy validation uses the same seed as dataset
construction. Update _assert_dataset_invariants and _assert_calib_columns to
accept and forward the seed, then pass build_dataset’s decoy_fold_seed at the
call site; use it in the decoy_calib call instead of FOLD_RANDOM_VALUES[0].
In `@tests/unit/test_split_table_schema.py`:
- Around line 172-183: Rename
test_calib_carve_is_deterministic_and_seed_sensitive and revise its docstring to
describe only deterministic output and the frozenset type check, removing the
unsupported claim that a different seed produces different clusters. Mention
that seed sensitivity is validated by
test_calib_clause_set_catches_a_reseeded_carve in tests/ml/test_no_leakage.py.
---
Nitpick comments:
In `@src/tbox_finder/stage2/dataset.py`:
- Around line 631-636: In build_dataset, add an early validation that the splits
DataFrame contains the calib column before downstream _carried processing. If it
is absent, raise an explicit error stating that split-table schema version 1.1
or later is required, while preserving existing behavior when the column is
present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d9e0c307-e8e1-4e16-b31e-72d9f754cf24
⛔ Files ignored due to path filters (7)
analyses/phase3_log.qmdis excluded by!**/*.qmddata/processed/audits/stage2_dataset_report.jsonis excluded by!data/**data/processed/splits/split_assignments.parquetis excluded by!**/*.parquet,!data/**data/processed/splits/split_assignments.provenance.jsonis excluded by!data/**data/processed/stage2_dataset.parquet.dvcis excluded by!data/**data/processed/stage2_dataset.provenance.jsonis excluded by!data/**docs/decisions/ADR-0004-split-and-leakage-policy.mdis excluded by!**/*.md,!docs/decisions/**
📒 Files selected for processing (9)
src/tbox_finder/data/window_dataset.pysrc/tbox_finder/splits.pysrc/tbox_finder/stage2/dataset.pytests/fixtures/stage2_dataset/expected.sha256tests/golden/test_stage2_dataset_golden.pytests/ml/test_no_leakage.pytests/unit/test_split_table_schema.pytests/unit/test_stage2_dataset.pytests/unit/test_window_dataset.py
Comment on lines
+1334
to
+1344
| sizes: Counter[int] = Counter() | ||
| stratum_of: dict[int, str] = {} | ||
| for i in eligible: | ||
| cid = int(cluster_id[i]) | ||
| sizes[cid] += 1 | ||
| if cid not in stratum_of and not masking.is_missing(resolved_genus[i]): | ||
| stratum_of[cid] = masking.row_text(resolved_genus[i]) | ||
|
|
||
| by_stratum: dict[str, list[int]] = defaultdict(list) | ||
| for cid in sizes: | ||
| by_stratum[stratum_of.get(cid, CALIB_UNASSIGNED_STRATUM)].append(cid) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the committed split table, then count eligible clusters holding >1 distinct genus.
fd -t f 'split_assignment' -x echo {}
python - <<'PY'
import glob
import pandas as pd
paths = glob.glob("**/*split*assignment*.parquet", recursive=True)
print("candidates:", paths)
for p in paths:
df = pd.read_parquet(p)
if not {"cluster_id", "resolved_genus", "nested_train", "source", "fold_random"} <= set(df.columns):
continue
pool = df[(df["source"] == "corpus") & df["nested_train"].astype(bool) & (df["fold_random"] == "train")]
n = pool.groupby("cluster_id")["resolved_genus"].nunique(dropna=True)
print(p, "clusters with >1 genus:", int((n > 1).sum()), "of", len(n))
PYRepository: bioedca/tbox-finder
Length of output: 530
🏁 Script executed:
set -eu
printf '%s\n' '--- relevant source and tests ---'
sed -n '1260,1370p' src/tbox_finder/splits.py
rg -n -C 4 'calib_cluster_ids|ignores_row_order|clade_crossing_cluster|resolved_genus' src tests data README.md 2>/dev/null | head -n 260
printf '%s\n' '--- available parquet readers ---'
command -v parquet-tools || true
command -v duckdb || true
python3 - <<'PY'
for name in ("pyarrow", "polars", "duckdb", "fastparquet"):
try:
mod = __import__(name)
print(name, "available", getattr(mod, "__version__", ""))
except Exception as exc:
print(name, "unavailable:", type(exc).__name__)
PY
printf '%s\n' '--- parquet file sizes ---'
find data -type f -name '*split*assignment*.parquet' -printf '%p %s bytes\n' 2>/dev/null || trueRepository: bioedca/tbox-finder
Length of output: 24660
🏁 Script executed:
set -eu
printf '%s\n' '--- cluster and clade-crossing construction ---'
sed -n '740,835p' src/tbox_finder/splits.py
printf '%s\n' '--- calibration fixture and row-order test ---'
sed -n '1,220p' tests/unit/test_split_table_schema.py
printf '%s\n' '--- standalone order-dependence probe ---'
python3 - <<'PY'
from collections import Counter, defaultdict
def strata(rows):
sizes = Counter()
stratum_of = {}
for cid, genus in rows:
sizes[cid] += 1
if cid not in stratum_of and genus is not None:
stratum_of[cid] = genus
by = defaultdict(list)
for cid in sizes:
by[stratum_of.get(cid, "__unassigned__")].append(cid)
return dict(stratum_of), {k: sorted(v) for k, v in by.items()}
rows = [(7, "GenusB"), (7, "GenusA"), (8, "GenusC")]
forward = strata(rows)
reverse = strata(list(reversed(rows)))
print("forward:", forward)
print("reverse:", reverse)
print("same:", forward == reverse)
# The proposed reduction is invariant for the same content.
def min_strata(rows):
names = defaultdict(set)
for cid, genus in rows:
if genus is not None:
names[cid].add(genus)
return {cid: min(values) for cid, values in names.items() if values}
print("min forward:", min_strata(rows))
print("min reverse:", min_strata(list(reversed(rows))))
print("min same:", min_strata(rows) == min_strata(list(reversed(rows))))
PY
printf '%s\n' '--- parquet strings related to schema and clade flags ---'
strings data/processed/splits/split_assignments.parquet | rg -n 'cluster_id|resolved_genus|clade_crossing_cluster|nested_train|fold_random' | head -n 40 || trueRepository: bioedca/tbox-finder
Length of output: 15546
Make the stratum assignment independent of row order.
stratum_of[cid] selects the first non-null resolved_genus. If an eligible cluster contains multiple genera, row reordering changes the calibration carve. The current row-order test does not cover this case because its fixture is genus-homogeneous. Collect all non-null genera per cluster and select a deterministic value, such as min(names). Add a heterogeneous-cluster test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tbox_finder/splits.py` around lines 1334 - 1344, Update the stratum
assignment around stratum_of so each eligible cluster collects all non-missing
resolved_genus values and assigns a deterministic representative, such as the
minimum name, instead of retaining the first encountered value. Preserve
CALIB_UNASSIGNED_STRATUM for clusters without any non-missing genus, and add a
test covering a heterogeneous cluster whose calibration result remains unchanged
when eligible rows are reordered.
Comment on lines
+172
to
+183
| def test_calib_carve_is_deterministic_and_seed_sensitive(): | ||
| """Same seed ⇒ same clusters; a different seed ⇒ different ones. | ||
|
|
||
| The second half is what makes the CI identity clause meaningful: if the draw were | ||
| seed-insensitive, re-deriving it would agree with *any* committed column and the | ||
| check would be a tautology. | ||
| """ | ||
| pytest.importorskip("pandas") | ||
| cols = _carve_cols(splits.build_split_table(_interim_frame())) | ||
| assert splits.calib_cluster_ids(**cols) == splits.calib_cluster_ids(**cols) | ||
| other = splits.calib_cluster_ids(**cols, seed=splits.CALIB_CARVE_SEED + 1) | ||
| assert isinstance(other, frozenset) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The test name and docstring claim seed sensitivity that the assertions do not check.
The docstring states "a different seed ⇒ different ones", and the name says seed_sensitive. The only assertion for the reseeded draw is isinstance(other, frozenset). On this eight-row fixture the two seeds can select the same clusters, because the Frankia stratum holds exactly one cluster and is always taken. The real seed-sensitivity check lives in tests/ml/test_no_leakage.py::test_calib_clause_set_catches_a_reseeded_carve.
Align the name and docstring with what is asserted, and point to the test that does check sensitivity.
💚 Proposed fix
-def test_calib_carve_is_deterministic_and_seed_sensitive():
- """Same seed ⇒ same clusters; a different seed ⇒ different ones.
-
- The second half is what makes the CI identity clause meaningful: if the draw were
- seed-insensitive, re-deriving it would agree with *any* committed column and the
- check would be a tautology.
- """
+def test_calib_carve_is_deterministic_and_accepts_an_alternate_seed():
+ """Same seed ⇒ same clusters; an alternate seed still yields a valid draw.
+
+ Seed *sensitivity* is asserted on the real partition, where the pool is large
+ enough for two seeds to differ:
+ ``tests/ml/test_no_leakage.py::test_calib_clause_set_catches_a_reseeded_carve``.
+ This fixture holds one single-cluster stratum that every seed must take, so an
+ inequality assertion here would be fixture-dependent.
+ """📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_calib_carve_is_deterministic_and_seed_sensitive(): | |
| """Same seed ⇒ same clusters; a different seed ⇒ different ones. | |
| The second half is what makes the CI identity clause meaningful: if the draw were | |
| seed-insensitive, re-deriving it would agree with *any* committed column and the | |
| check would be a tautology. | |
| """ | |
| pytest.importorskip("pandas") | |
| cols = _carve_cols(splits.build_split_table(_interim_frame())) | |
| assert splits.calib_cluster_ids(**cols) == splits.calib_cluster_ids(**cols) | |
| other = splits.calib_cluster_ids(**cols, seed=splits.CALIB_CARVE_SEED + 1) | |
| assert isinstance(other, frozenset) | |
| def test_calib_carve_is_deterministic_and_accepts_an_alternate_seed(): | |
| """Same seed ⇒ same clusters; an alternate seed still yields a valid draw. | |
| Seed *sensitivity* is asserted on the real partition, where the pool is large | |
| enough for two seeds to differ: | |
| ``tests/ml/test_no_leakage.py::test_calib_clause_set_catches_a_reseeded_carve``. | |
| This fixture holds one single-cluster stratum that every seed must take, so an | |
| inequality assertion here would be fixture-dependent. | |
| """ | |
| pytest.importorskip("pandas") | |
| cols = _carve_cols(splits.build_split_table(_interim_frame())) | |
| assert splits.calib_cluster_ids(**cols) == splits.calib_cluster_ids(**cols) | |
| other = splits.calib_cluster_ids(**cols, seed=splits.CALIB_CARVE_SEED + 1) | |
| assert isinstance(other, frozenset) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_split_table_schema.py` around lines 172 - 183, Rename
test_calib_carve_is_deterministic_and_seed_sensitive and revise its docstring to
describe only deterministic output and the frozenset type check, removing the
unsupported claim that a different seed produces different clusters. Mention
that seed sensitivity is validated by
test_calib_clause_set_catches_a_reseeded_carve in tests/ml/test_no_leakage.py.
…thread the decoy seed Two real defects, both found by CodeRabbit CLI on PR #94. 1. `CALIB_STRATUM_COLUMN` was decorative. It is a pinned constant AND the value the provenance sidecar reports as `calib.stratum_column`, but the carve read `table["resolved_genus"]` by name and `calib_cluster_ids` took a `resolved_genus=` keyword. Changing the constant would have left the committed provenance claiming a stratification the carve never performed — a fabricated provenance value (CLAUDE.md 10.3) with nothing failing. The parameter is now `stratum` and every caller (the writer, the CI re-derivation, the unit fixtures) selects the column through the constant. Verified load-bearing by execution: on the real table the carve moves 859 -> 526 records when the constant is pointed at `resolved_order`. 2. The calib guard re-derived with the module-default seed. `build_dataset` takes `decoy_fold_seed` and threads it into `decoy_calib`, but `_assert_calib_columns` called `decoy_calib` with the default -- so a caller passing any other seed had their rows compared against a different hash: the guard would fire on a correct build and could never fire on a wrong one. The seed is now threaded through `_assert_dataset_invariants` into the guard, and the guard re-derives over EVERY parentless row rather than filtering to calib=True first, so a decoy wrongly left OUT -- the count-preserving direction -- is caught too. Both fixes are sabotage-verified, and one of the tests had to be fixed first: the initial version used `DECOY_FOLD_SEED + 1`, but the fixture has only two parentless decoys and at rate 0.0469 almost every seed leaves both False, so the test PASSED with the bug reverted -- structurally blind. It now uses a searched seed (20260737) that provably disagrees with the default, asserts that disagreement explicitly so it cannot go vacuous, and fails when the fix is reverted. Same trap in the other direction for the stratum test: the unit fixture had one genus per order, so genus- and order-stratification coincided; the fixture now carries two genera inside one order (genus -> 3, order -> 2). Artifacts are byte-identical (split table sha256 d51252ee..., stage2 digest c5a2c251...) -- these are correctness fixes to guards and plumbing, not to the carve. ruff + black clean, snakemake --lint clean, no-leakage still 57/0. Claude-Session: https://claude.ai/code/session_01Tr67opmzHJAGBJPAb2aTdX
…ity, not by rate
Two findings, both applied; one a real test weakness, one defensive.
1. REAL. `test_decoy_calib_is_independent_of_the_fold_draw` asserted independence
through a rate band (0.5x-2.0x DECOY_CALIB_RATE), but the exact bug it names --
implementing `calib` as the low tail of the SAME unit interval `decoy_fold`
partitions -- yields 0.0536 among train-fold decoys (DECOY_CALIB_RATE / 0.80),
which is inside any band loose enough to survive binomial noise. Measured
first, then replaced with an assertion by IDENTITY against the shared-domain
set. Sabotage-verified: collapsing the hash domain to `{seed}:{row_id}` now
fails that test (and the seed test with it); restoring passes 29/29.
2. DEFENSIVE. `_calib_provenance` counted `calib` over all rows while
`n_eligible_records` counted corpus rows only. Measured on the committed
table this is a no-op -- 0 non-corpus calib rows, and `_assert_calib_disjoint`
refuses them before the provenance is written -- but the numerator and
denominator of `realised_fraction` now range over the same population, so the
number cannot go wrong if that assertion is ever reordered or relaxed.
Dev-log stanza gains both review rounds, including the admission that two of my
own tests were structurally blind and that sabotage, not reading, is what caught
them.
Artifacts byte-identical (split table sha256 d51252ee..., stage2 digest
c5a2c251...). ruff + black clean; no-leakage 57/0.
Claude-Session: https://claude.ai/code/session_01Tr67opmzHJAGBJPAb2aTdX
This was referenced Aug 3, 2026
Merged
Merged
P3-08: the with/without-aux ablation on the calibrated binary head + the Stage-2 score producer
#101
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ADR-0005 D11 pins the recalibration stack as train → temperature-scale on a disjoint calibration split → prior-shift, but ADR-0004's fold vocabulary had no calibration fold. A11 (P2-13) recorded the absence, fitted its non-gated Stage-1
Ton a seeded half of theselection_valrung as a stopgap, and named P3-02 as the step that carves the real one. This PR signs ADR-0004 A7 (bioedca, 2026-08-01) and builds it.The carve
calibis drawn fromsource == "corpus" ∧ nested_train ∧ fold_random == "train" ∧ cluster ∉ selection_val(5,034 records / 2,526 clusters) by a genus-stratified, whole-cluster, seeded draw (CALIB_CARVE_SEED = 20260801, fraction 0.10) → 859 records / 431 clusters / 23 orders / 13 phyla (realised 17.1 %).fold_random == "train"conjunct makes disjointness from the graded split structural —fold_randomis whole-cluster assigned (measured: 0 clusters span >1 non-null value) — and removes ADR-0004 A6'sgate4_evalpopulation in full.selection_val-style draw reaches only 6 of 25 orders where stratification reaches 23.The negative side (the decision P3-01 explicitly flagged)
The committed table has no negative rows (the A4/b2 blind spot), so a positives-only
calibcannot calibrate a binary head. Parentless decoys are admitted from the train portion only, atDECOY_CALIB_RATE = 0.0469, by a keyed hash under a distinct":calib:"domain prefix — a second draw rather than a 4-way widening ofdecoy_fold, which would move decoys across the train/val/test boundary and silently change a partition P3-01 already committed.nested_trainstays null for them.A fork this step exposed
window_datasetcarried a hand-typed copy ofFOLD_SCHEME_COLUMNSthat went one column stale with nothing failing. It is now derived assplits.FOLD_SCHEME_COLUMNS − STAGE2_ONLY_FOLD_COLUMNS— content and order unchanged, omission named and asserted.Validation — PASS
Full-corpus no-leakage green: 57 passed / 0 failed (was 49). On the real partition, calib ∩ {val, test} = 0, ∩
selection_val= 0, ∩gate4_eval= 0, ∩ designated LOO holdout = 0, ∩ clade-crossing = 0, straddling clusters = 0. The 22 cluster-mates left outsidecalibare allnested_role == "dropped"; 0 arenested_train.The CI clause re-derives the carve and asserts set identity rather than reading the committed boolean back. Seven targeted sabotages on the real table each bite the intended clause, and
loosening_admits_the_refusedis non-vacuous at baseline (pool 5,034 → 6,366 without the conjunct).ruff+blackclean at the CI-pinned versions;snakemake --lint/-nclean. Both artifacts were regenerated after the reformat and reproduce byte-identical digests.Reported, not tuned
Realised calib prevalence is 0.7888 vs the in-distribution test split's 0.7727 (+1.6 pp): the keyed hash drew 168 parentless decoys where the rate's expectation is 188.9 (−1.6 σ), and 62 rather than 64 inherited.
DECOY_CALIB_RATEis not re-tuned — the rate is the pinned quantity, the prevalence is a measurement (CLAUDE.md §10.3). Inherit-only would sit 15.4 pp off.Disclosed limitation.
calibis carved from insidenested_train, so its clade support is narrower than the test split's by construction (23 orders / 13 phyla vs 48 / 12). Carried as a calibration-transfer caveat for P3-10, not corrected by reweighting.https://claude.ai/code/session_01Tr67opmzHJAGBJPAb2aTdX
Summary by CodeRabbit
New Features
Bug Fixes
Tests