Skip to content

P3-10: GATE-2's in-distribution ECE gate + the leave-clade-out drift read - #103

Merged
bioedca merged 8 commits into
mainfrom
p3-10-gate2-ece
Aug 5, 2026
Merged

bioedca merged 8 commits into
mainfrom
p3-10-gate2-ece

Conversation

@bioedca

@bioedca bioedca commented Aug 4, 2026

Copy link
Copy Markdown
Owner

P3-10 — GATE-2's Phase-3 half: the in-distribution ECE gate on the named posterior, plus the leave-clade-out and deployment-prevalence reads beside it.

PRD §2.3 (GATE-2) · §12 · §18.1 · §9.2 — ADR-0005 D11 (named posterior; 15 equal-mass debiased bins; ECE ≤ 0.05, blinded-frozen, gated at P3 exit) + D13 & Amendment A2 (a distinct small-N-robust OOD estimator, bootstrap CIs, OOD_ECE_MIN_N = 20) — ADR-0004 D5 (nested leave-one-order-out fold) + A7.

✅ GATE-2 (P3 half) PASSES — ECE 0.005662 ≤ 0.05

gated statistic 0.005662 (debiased, 15 equal-mass bins)
pre-registered ceiling 0.05 (blinded-frozen, ADR-0005 D11)
margin 8.8× under; CI upper bound 5.3× under
cluster-blocked 95% CI [0.002700, 0.009410] (2,275 blocks, B = 2,000)
plug-in (un-debiased) 0.008378
graded object named posterior — temperature-scaled, PRE prior-shift
population test rung, 3,045 rows / 2,353 pos / prevalence 0.7727
temperature T = 1.140627, fitted on the disjoint 1,089-row calib carve

It reproduces P3-08's reading_absolute.observed_ece exactly. The plug-in is reported beside the gated value because P3-09 measured the D11 debias term to under-state real error at small n — both clearing 0.05 by an order of magnitude is what makes the pass independent of that concern.

The step had to build its own OOD population

P3-08 scored only the random split's three rungs, so of the 30 ADR-0004 D5 leave-one-order-out units only 26 appeared at all and 5 cleared min-N — a coverage figure that would have sat in a phase-exit artifact contradicting Amendment A2's ex ante 30-unit footprint while reading like a finding about the corpus. gate2 score-loo scores the full designated holdout with the production arm — 9,345 rows / 30 orders / 8,641 positives, all 30 clearing the floor — reusing stage2.eval's checkpoint loader and scorer rather than forking them. ~28 min, laptop 4060, batch 4, flash-attention-2 (the backend the arm trained under).

Its OOD-ness is measured, and one of the two checks is invisible at row level. Re-deriving P3-06's admission predicate from the split table reproduces 9,059 training rows exactly; against it the holdout has 0 shared rows and 0 shared orders (19 trained vs 30 held out). The producer refuses to run if either is non-zero.

The drift is phylum-structured — a step function, not a gradient

Macro-average OOD ECE 0.2113 (95% CI [0.0984, 0.3250]), and it describes neither mode:

group orders OOD ECE range
Actinobacteria 9 0.5923 – 0.7991
Firmicutes · Tenericutes · Chloroflexi · Deinococcus-Thermus · Synergistetes · Proteobacteria 21 0.0007 – 0.1151

The two sets do not overlap — 9/9 high-drift orders are Actinobacteria, 0/21 others exceed 0.115. Against a ~90%-Firmicutes training corpus, calibration fails at the phylum boundary rather than degrading smoothly with rank. ood.by_phylum carries this as a descriptive regrouping of the same per-unit estimates: no pooled phylum estimate, no CI, no verdict. It also vindicates PRD §12's macro-not-micro rule — Lactobacillales alone is 61.6% of held-out positives at ECE 0.0034, so a row-weighted summary would have reported near-perfect transfer.

The gated margin is bin-concentrated

The posterior is near-separated, so under equal-mass binning 12 of the 15 bins carry accuracy exactly 0 or 1 — rows at p < 1e-4 or p > 0.995, where there is nothing to get wrong — while one bin spans p = 4.5e-5 to 0.9956 (99.6% of the probability axis) and absorbs the whole transition region. That bin's raw gap is 0.0985, about 2× the gate threshold; at weight 1/15 it supplies 83.1% of the reported 0.005662.

The pass is honest and its evidence base is narrow — the same shape as P3-08's single misclassified calib row. gate.bin_concentration records the share, the saturated-bin count and the widest span, and a disclosure quotes the measured share. Nothing is gated on any of it. The reliability figure draws each bin as a segment over the range it actually covers, does not join them, and labels the overplotted piles — joining fifteen centres that sit in two piles plus one bridge would draw a well-sampled-looking curve through ranges where nothing was measured.

Two values had to stay unpinned, and the validator enforces it

  1. D13 condition (i)'s drift bound does not exist — absent from PRD.md, ADR-0005, ADR-0006, conf/ and src/; D18's delegation map assigns only the min-N floor. Condition (ii) is answered per unit, (i) is recorded unpinned, (iii) is a P4 quantity, and calibrated_negative_pass stays null. A two-of-three verdict reads exactly like a three-of-three one.
  2. No deployment prior is pinned~10³–10⁴ : 1 is prose only (PRD §11), so the prior-shifted ECE is a band sweep (0.5642 / 0.6814 / 0.7372), non-gated and miscalibrated by construction — which is why D11 gates the pre-shift object.

validate_report refuses a report that pins either. Same shape as P3-07's π_deploy and P3-08's τ. No ADR amended, no §7 sign-off sought — nothing pinned changed.

Two defects the shipped path had, both found by running it

  1. build_provenance hashes every path in outputs, and neither output exists when it is called — the first full run would have died at the final line after ~40 min of bootstrapping. Outputs are now declared names under extra.
  2. prior_shift returns shifted logits (deliberately — additive there), and feeding them to binned_ece as probabilities reported an "ECE" of 6.29. The sweep now routes through the shipped calibrated_posterior, with a guard refusing anything outside [0, 1].

A cheap --max-units 3 --ood-n-boot 5 dry run of the whole entry point surfaced both.

is_science is derived, not asserted

GATE-4's report can hardcode it because its incomplete path raises and writes no file; this one always writes, so --max-units would otherwise land a truncated grade at the phase-exit path flagged as real science. It is now the conjunction of the three completeness clauses, re-derived by validate_report.

Validation

  • Report re-validates clean under the shipped code — all 19 clauses re-derive identically, overall_pass = true.
  • 52 tests/unit/test_gate2.py (numpy-only, runs in bare CI) + the ood_ece golden P3-09 deferred here, blocked on each fixture record's real taxonomic order (the 50 GC-background decoys, which have no source record, are excluded rather than assigned one).
  • Full unit+golden+ml: 3,898 passed / 14 failed — the 14 exactly a matched baseline worktree's set at main (comm empty both ways).
  • 47 sabotages, each RED against its NAMED test, each restored byte-identically. Two did not bite on the first pass and both gaps were real: an all-TRUE fixture made overall_pass = all(clauses) indistinguishable from = True, and the disclosure test matched a phrase that survived deleting the clause that introduced it.
  • ruff 0.15.0 + black 25.11.0 + snakemake --lint clean; snakemake -n gate2_ece resolves stage2_dataset → stage2_scores_loo → gate2_ece.

Notes

  • Path drift, recorded not taken silently: the module lands at src/tbox_finder/calib/gate2.py, not imp.md's calibration/gate2.py — the same correction as P3-07 and P3-09.
  • Three items now stand for the P3-exit ADR round: D11's missing degenerate-limit rule (P3-08), the D11 debias term's measured saturation (P3-09), and D13's unpinned drift bound (this step).

Summary by CodeRabbit

  • New Features

    • Added GATE-2 calibration evaluation for in-distribution and leave-one-group-out data.
    • Added temperature calibration, reliability metrics, confidence intervals, prior-shift analysis, and minimum-sample checks.
    • Added automated calibration reports, figure data, reliability plots, and OOD summaries.
    • Added workflow support for scoring, grading calibration, and rendering figures.
  • Bug Fixes

    • Improved validation of incomplete, inconsistent, truncated, or non-disjoint evaluation data.
    • Added safeguards for unsupported drift conditions and rescoring disagreements.
  • Tests

    • Added comprehensive coverage for calibration, OOD evaluation, report validation, and edge cases.

…out drift read

GATE-2's Phase-3 half PASSES: in-distribution ECE 0.005662 <= 0.05 (8.8x under;
cluster-blocked 95% CI [0.002700, 0.009410], B = 2,000) on the named posterior —
temperature-scaled, PRE prior-shift — on the `test` rung at its own prevalence
(3,045 rows / 2,353 pos / 2,275 clusters), T = 1.140627 fitted on the disjoint
1,089-row calib carve. Reproduces P3-08's number exactly; the un-debiased plug-in
(0.008378) is reported beside it, so the pass does not rest on the D11 debias term
P3-09 measured to under-state.

Inputs:  reports/p3/stage2_scores.json (P3-08); data/processed/stage2_dataset.parquet
         (376ad12b…); checkpoints/stage2_rinalmo/aux1.0_lr1e-4; calib.recalibrate
         (P3-07), calib.ece + eval.resample (P3-09), metrics.binned_ece (P0-31),
         coverage.OOD_ECE_MIN_N = 20 (ADR-0005 A2).
Outputs: src/tbox_finder/calib/gate2.py; workflow/rules/calibration.smk (3 rules);
         reports/gate2_p3_ece.json; reports/p3/gate2_figure_data.json;
         reports/p3/stage2_scores_loo.json; figures/calib/gate2_{reliability,
         ood_by_order}.png; tests/unit/test_gate2.py (52); the ood_ece golden P3-09
         deferred here; paper/manuscript.qmd #sec-gate2 + @fig-gate2.

THE STEP HAD TO BUILD ITS OWN OOD POPULATION. P3-08 scored only the random split's
three rungs, so of the 30 ADR-0004 D5 leave-one-order-out units only 26 appeared and
5 cleared min-N — a coverage figure that would have contradicted ADR-0005 A2's ex
ante 30-unit footprint while reading like a finding. `gate2 score-loo` scores the
full designated holdout with the production arm (9,345 rows / 30 orders / 8,641
positives, all 30 clearing the floor), reusing stage2.eval's loader and scorer. It
refuses to run unless the holdout is disjoint from training by row AND by order;
measured 0 shared rows and 0 shared orders against a re-derived 9,059-row training
set. The order-level check is the one a row check cannot see.

THE DRIFT IS PHYLUM-STRUCTURED. Macro-average OOD ECE 0.2113 (95% CI [0.0984,
0.3250]) conceals a clean bimodality: the 9 Actinobacteria orders read 0.5923–0.7991,
the 21 non-Actinobacteria orders read 0.0007–0.1151, and the two sets do not overlap.
`ood.by_phylum` carries this as a descriptive regrouping — no pooled estimate, no CI,
no verdict. Lactobacillales alone is 61.6% of held-out positives at ECE 0.0034, which
is why PRD §12 macro-averages rather than micro-averages.

THE GATED MARGIN IS BIN-CONCENTRATED. The posterior is near-separated: 12 of 15
equal-mass bins carry accuracy exactly 0 or 1, while one bin spans p = 4.5e-5 to
0.9956 and supplies 83.1% of the reported ECE. Recorded in gate.bin_concentration
plus a disclosure; nothing is gated on it. The reliability figure draws bins as
spans, does not join them, and labels the overplotted piles.

TWO VALUES STAY UNPINNED and validate_report refuses either being pinned: D13
condition (i)'s drift bound does not exist repo-wide (D18 delegates only min-N), so
calibrated_negative_pass stays null; and no deployment prior is pinned, so the
prior-shifted ECE is a band sweep (0.5642 / 0.6814 / 0.7372). No ADR amended, no §7
sign-off — nothing pinned changed.

Two defects found by running the shipped path, not reading it: build_provenance
hashes its `outputs` (which do not exist yet — would have died after ~40 min of
bootstrapping), and prior_shift returns logits, so the sweep reported an "ECE" of
6.29 before routing through calibrated_posterior. `is_science` is now derived from
the three completeness clauses rather than hardcoded, so a --max-units smoke cannot
present as a full grade.

Validation: report re-validates clean under the shipped code, all 19 clauses
re-deriving identically, overall_pass = true. Full unit+golden+ml 3,898 passed /
14 failed — the 14 exactly a matched baseline worktree's set at main (comm empty
both ways). 47 sabotages, each RED against its NAMED test, each restored
byte-identically. ruff 0.15.0 + black 25.11.0 + snakemake --lint clean.

Path drift recorded: the module lands at calib/gate2.py, not imp.md's
calibration/gate2.py — same correction as P3-07 and P3-09.
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 19 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c9571e8a-77aa-4f80-8aaf-c8489374ee09

📥 Commits

Reviewing files that changed from the base of the PR and between ae6ea40 and 7e65e63.

⛔ Files ignored due to path filters (1)
  • analyses/phase3_log.qmd is excluded by !**/*.qmd
📒 Files selected for processing (4)
  • reports/gate2_p3_ece.json
  • reports/p3/gate2_figure_data.json
  • src/tbox_finder/calib/gate2.py
  • tests/unit/test_gate2.py
📝 Walkthrough

Walkthrough

Adds GATE-2 P3 calibration grading with in-distribution ECE gating, non-gated OOD estimation, report validation, figure generation, tests, and Snakemake workflow rules.

Changes

GATE-2 calibration

Layer / File(s) Summary
Population reconstruction and score loading
src/tbox_finder/calib/gate2.py, tests/unit/test_gate2.py, workflow/rules/calibration.smk
Reconstructs training and leave-one-order-out populations, checks row and order disjointness, validates score files, and generates OOD scores.
Calibration and OOD estimators
src/tbox_finder/calib/gate2.py, tests/unit/test_gate2.py, tests/ml/test_eval_gate.py, tests/fixtures/eval_gate_sample/expected.json
Computes temperature-scaled in-distribution ECE, prior-shift summaries, and non-gated leave-one-order-out OOD ECE with bootstrap intervals and admissibility checks.
Report assembly and validation
src/tbox_finder/calib/gate2.py, reports/gate2_p3_ece.json, tests/unit/test_gate2.py
Builds and validates clauses, disclosures, completeness metadata, rescoring metadata, and the generated GATE-2 report.
Rescoring, figures, and workflow execution
src/tbox_finder/calib/gate2.py, reports/p3/gate2_figure_data.json, workflow/rules/calibration.smk
Adds production OOD rescoring, figure-data projection, reliability and OOD plots, CLI commands, and workflow rules for scoring, grading, and plotting.

Estimated code review effort: 5 (Critical) | ~90 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Snakemake
  participant gate2_py
  participant ScoreFiles
  participant GATE2Report
  participant Figures
  Snakemake->>gate2_py: Run leave-one-order-out scoring
  gate2_py->>ScoreFiles: Load and validate scores
  Snakemake->>gate2_py: Run calibration grading
  gate2_py->>GATE2Report: Write validated report and figure data
  Snakemake->>gate2_py: Run figure rendering
  gate2_py->>Figures: Write reliability and OOD figures
Loading

Possibly related PRs

  • bioedca/tbox-finder#90: Provides temperature-scaling and shared reliability-bin/ECE functionality used by GATE-2.
  • bioedca/tbox-finder#94: Introduces the calibration split and disjoint calibration assignment used for temperature fitting.
  • bioedca/tbox-finder#102: Introduces the OOD-ECE estimator and shared block-bootstrap resampler used by GATE-2.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 59.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main changes: the GATE-2 in-distribution ECE gate and leave-clade-out drift assessment.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch p3-10-gate2-ece

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (8)
src/tbox_finder/calib/gate2.py (4)

1301-1304: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the stale legend comment.

Two comments describe the legend placement and they contradict each other. The first says the legend goes below the axes; the second says above. The code places it above the axes. Keep the second comment only.

♻️ Proposed fix
-    # Below the axes, not "best": the units are sorted by drift, so the largest values sit
-    # in the upper right and any in-axes legend lands on top of them.
     # ABOVE the axes: the units are sorted by drift, so the largest values sit in the upper
     # right and an in-axes legend lands on them, while below the axes it lands on the label.
🤖 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/calib/gate2.py` around lines 1301 - 1304, Remove the stale
first legend-placement comment near the axes plotting logic, preserving only the
second comment that states the legend is placed above the axes. Do not change
the code or the retained explanation.

360-374: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Include rungs in the parallel-array check.

load_scores validates row_ids, logits, and labels lengths, but not rungs. A rungs array shorter than row_ids silently grades a subset of the test rung; a longer one raises IndexError later inside grade_in_distribution. The completeness clause catches the truncation case downstream, so this is a hardening step, not a live defect. Add the length to the same invariant so the failure names the score file.

♻️ Proposed check
     labels = [int(v) for v in payload["labels"]]
     if not (len(row_ids) == len(logits) == len(labels)):
         raise ValueError(
             f"{path}: parallel arrays disagree — row_ids={len(row_ids)}, "
             f"logits={len(logits)}, labels={len(labels)}"
         )
+    rungs = [str(v) for v in payload.get("rungs", [])] or None
+    if rungs is not None and len(rungs) != len(row_ids):
+        raise ValueError(
+            f"{path}: rungs disagrees with the other arrays — rungs={len(rungs)}, "
+            f"row_ids={len(row_ids)}"
+        )
     if len(set(row_ids)) != len(row_ids):
         raise ValueError(f"{path}: row_ids are not unique, so a join back would fan out")
     return {
         "row_ids": row_ids,
         "logits": logits,
         "labels": labels,
-        "rungs": [str(v) for v in payload.get("rungs", [])] or None,
+        "rungs": rungs,
🤖 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/calib/gate2.py` around lines 360 - 374, Update the
parallel-array validation in load_scores to include the normalized rungs array
length alongside row_ids, logits, and labels. Ensure the ValueError reports the
rungs count and preserve the existing optional/None behavior for missing or
empty rungs.

1473-1515: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

The grade path reads and hashes the dataset three times.

main calls _read_split_table at line 1473, loo_holdout_rows at line 1484, and gate_rung_row_ids at line 1515. Each call reads the parquet and computes PROV.sha256_file on the same file. For a 30k-row table the cost is small, but the work is fully redundant and the three hashes must agree by construction. Read once and pass the rows to the pure helpers.

holdout_from_rows already accepts rows, and gate_rung_row_ids needs only rungs_for_rows, so a rows-taking variant of it removes the third read.

🤖 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/calib/gate2.py` around lines 1473 - 1515, The grade path in
main currently rereads and rehashes the dataset through _read_split_table,
loo_holdout_rows, and gate_rung_row_ids. Reuse the rows loaded by
_read_split_table by passing them to holdout_from_rows, and add or use a
rows-taking gate-rung helper that applies rungs_for_rows directly; update main
to call that helper with split_rows while preserving the existing results.

423-425: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Name the failure when a graded row has no block label.

Line 425 indexes blocks_by_row directly. If the in-distribution score file carries a row_id that the split table does not, this raises a bare KeyError with only the row id. The OOD path raises an explicit ValueError for the same class of mismatch at lines 622-627. Use the same shape here so the operator sees which population disagreed.

♻️ Proposed fix
     y = [int(scores["labels"][i]) for i in idx]
     p = [posterior[i] for i in idx]
-    keys = [blocks_by_row[scores["row_ids"][i]] for i in idx]
+    missing = sorted({scores["row_ids"][i] for i in idx} - set(blocks_by_row))
+    if missing:
+        raise ValueError(
+            f"{len(missing)} graded row(s) are absent from the split table's block map "
+            f"(e.g. {missing[:5]!r}) — the block bootstrap would then resample rows whose "
+            "exchangeability unit is unknown"
+        )
+    keys = [blocks_by_row[scores["row_ids"][i]] for i in idx]
🤖 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/calib/gate2.py` around lines 423 - 425, Update the graded-row
key lookup in the surrounding calibration function to catch a missing row_id in
blocks_by_row and raise an explicit ValueError matching the OOD path’s error
shape, including the in-distribution population context and offending row_id.
Preserve normal key lookup behavior for rows present in the split table.
tests/unit/test_gate2.py (1)

547-592: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer monkeypatch.setattr over manual global mutation.

The test reassigns COV.OOD_ECE_MIN_N and ECE.OOD_ECE_MIN_N and restores them in finally. The restore is correct. monkeypatch.setattr removes the manual bookkeeping and restores the attributes even if an assertion raises outside the try block during a later edit.

🤖 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_gate2.py` around lines 547 - 592, Update
test_the_min_n_floor_is_the_pinned_constant_and_the_boundary_tracks_it to accept
pytest’s monkeypatch fixture and use monkeypatch.setattr for both
COV.OOD_ECE_MIN_N and ECE.OOD_ECE_MIN_N. Remove the manual original-value
restoration and try/finally block, while preserving the existing assertions and
ensuring both attributes are patched together before calling ECE.ood_ece.
tests/ml/test_eval_gate.py (1)

569-573: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

The regression test recomputes the whole smoke fixture.

Line 573 calls _compute_smoke_metrics() a second time. test_eval_gate_matches_committed_expectation already computes it. The comment at lines 438-441 states that _OOD_N_BOOT is 60 because the leave-one-out kernel is O(n^2) per replicate and this tier has a wall-clock budget. Two full computations spend that budget twice. Cache the payload in a module-scoped fixture, or in a functools.lru_cache-wrapped helper, so both tests share one computation.

🤖 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/ml/test_eval_gate.py` around lines 569 - 573, Avoid recomputing the
smoke fixture in _compute_smoke_metrics by introducing a module-scoped fixture
or functools.lru_cache-wrapped helper that caches its payload. Update both
test_eval_gate_matches_committed_expectation and the regression test using
ood_ece to consume the shared result, preserving the existing _OOD_N_BOOT=60
configuration and test behavior.
workflow/rules/calibration.smk (2)

20-52: 🧹 Nitpick | 🔵 Trivial

Consider declaring the GPU requirement as a resource.

The docstring states this rule needs one GPU and about 28 minutes. The rule declares no threads or resources, so Snakemake can schedule it beside another GPU rule when --cores allows. A resources: gpu=1 declaration makes the constraint schedulable rather than documented only.

🤖 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 `@workflow/rules/calibration.smk` around lines 20 - 52, Add a Snakemake
resource declaration to the `stage2_scores_loo` rule requiring one GPU, using
the project’s established GPU resource key if one exists. Keep the existing
inputs, outputs, logging, conda environment, and shell command unchanged.

14-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The figures directory is declared twice.

_GATE2_FIGURES pins figures/calib/... at lines 14-17, and line 117 repeats figures/calib as a literal shell argument. A change to one does not follow the other, and the rule would then write outside its declared outputs. Derive the directory from the constant.

♻️ Proposed fix
+_GATE2_FIGURES_DIR = "figures/calib"
 _GATE2_FIGURES = [
-    "figures/calib/gate2_reliability.png",
-    "figures/calib/gate2_ood_by_order.png",
+    f"{_GATE2_FIGURES_DIR}/gate2_reliability.png",
+    f"{_GATE2_FIGURES_DIR}/gate2_ood_by_order.png",
 ]
     shell:
         "PYTHONPATH=src python -m tbox_finder.calib.gate2 plot-figures "
         "--figure-data {input.figure_data:q} "
-        "--figures-dir figures/calib >{log} 2>&1"
+        f"--figures-dir {_GATE2_FIGURES_DIR} >{{log}} 2>&1"

Confirm the f-string form is accepted inside your Snakemake shell: directive; if not, pass the directory through params: instead.

Also applies to: 114-117

🤖 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 `@workflow/rules/calibration.smk` around lines 14 - 17, Update the rule using
_GATE2_FIGURES so its shell command derives the figure output directory from
that constant instead of repeating the literal figures/calib path. Use a
Snakemake-supported f-string expression in shell:, or pass the derived directory
through params: if required, ensuring declared outputs and written files remain
aligned.
🤖 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 `@reports/gate2_p3_ece.json`:
- Around line 1939-1951: Normalize all recorded paths in gate2.py to
repository-relative POSIX paths before report serialization, including
provenance.extra.declared_outputs, the provenance.inputs key for
stage2_scores_loo.json, and scoring.loo_scores. Reuse the existing
repository-root/path normalization helper if available, ensure equivalent
input/output/score entries follow the same handling, then regenerate the report
artifact.

In `@src/tbox_finder/calib/gate2.py`:
- Around line 1174-1183: Update the legend label in the row-plotting loop to
avoid claiming every bin has rows[0]["n"] rows; either report the actual
minimum-to-maximum bin-count range from rows or remove the count while
preserving the existing single-label behavior for i == 0.
- Around line 1565-1574: Update the report-writing flow around validate_report
and _output_path so that when validation fails, the canonical report path is
removed before writing the invalid report. Apply the same valid/invalid
diversion to args.figure_data, writing figure data to its invalid destination
rather than overwriting the canonical artifact; preserve canonical outputs for
valid reports.

In `@tests/ml/test_eval_gate.py`:
- Around line 371-375: Add an assertion at the boundary where
`_detection_block_units` output is paired with detection labels, verifying the
unit vector length equals the label vector length before shared-index
processing. Update the `n_excluded_no_block == 53` accounting assertion near the
detection-count checks to document that this value depends on the current decoy
seed having no sampled source records among the three null-order records.

In `@tests/unit/test_gate2.py`:
- Around line 28-36: Move pytest.importorskip("numpy") before the module-level
tbox_finder imports in tests/unit/test_gate2.py so missing numpy skips
collection before those imports execute. Preserve the existing imported aliases,
and add the repository’s appropriate E402 suppression to the moved imports if
required by linting.

---

Nitpick comments:
In `@src/tbox_finder/calib/gate2.py`:
- Around line 1301-1304: Remove the stale first legend-placement comment near
the axes plotting logic, preserving only the second comment that states the
legend is placed above the axes. Do not change the code or the retained
explanation.
- Around line 360-374: Update the parallel-array validation in load_scores to
include the normalized rungs array length alongside row_ids, logits, and labels.
Ensure the ValueError reports the rungs count and preserve the existing
optional/None behavior for missing or empty rungs.
- Around line 1473-1515: The grade path in main currently rereads and rehashes
the dataset through _read_split_table, loo_holdout_rows, and gate_rung_row_ids.
Reuse the rows loaded by _read_split_table by passing them to holdout_from_rows,
and add or use a rows-taking gate-rung helper that applies rungs_for_rows
directly; update main to call that helper with split_rows while preserving the
existing results.
- Around line 423-425: Update the graded-row key lookup in the surrounding
calibration function to catch a missing row_id in blocks_by_row and raise an
explicit ValueError matching the OOD path’s error shape, including the
in-distribution population context and offending row_id. Preserve normal key
lookup behavior for rows present in the split table.

In `@tests/ml/test_eval_gate.py`:
- Around line 569-573: Avoid recomputing the smoke fixture in
_compute_smoke_metrics by introducing a module-scoped fixture or
functools.lru_cache-wrapped helper that caches its payload. Update both
test_eval_gate_matches_committed_expectation and the regression test using
ood_ece to consume the shared result, preserving the existing _OOD_N_BOOT=60
configuration and test behavior.

In `@tests/unit/test_gate2.py`:
- Around line 547-592: Update
test_the_min_n_floor_is_the_pinned_constant_and_the_boundary_tracks_it to accept
pytest’s monkeypatch fixture and use monkeypatch.setattr for both
COV.OOD_ECE_MIN_N and ECE.OOD_ECE_MIN_N. Remove the manual original-value
restoration and try/finally block, while preserving the existing assertions and
ensuring both attributes are patched together before calling ECE.ood_ece.

In `@workflow/rules/calibration.smk`:
- Around line 20-52: Add a Snakemake resource declaration to the
`stage2_scores_loo` rule requiring one GPU, using the project’s established GPU
resource key if one exists. Keep the existing inputs, outputs, logging, conda
environment, and shell command unchanged.
- Around line 14-17: Update the rule using _GATE2_FIGURES so its shell command
derives the figure output directory from that constant instead of repeating the
literal figures/calib path. Use a Snakemake-supported f-string expression in
shell:, or pass the derived directory through params: if required, ensuring
declared outputs and written files remain aligned.
🪄 Autofix

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: e99f5efd-2595-47b9-a929-9f33de8c3f5a

📥 Commits

Reviewing files that changed from the base of the PR and between 8301e0c and 5cba66a.

⛔ Files ignored due to path filters (4)
  • analyses/phase3_log.qmd is excluded by !**/*.qmd
  • figures/calib/gate2_ood_by_order.png is excluded by !**/*.png, !figures/**
  • figures/calib/gate2_reliability.png is excluded by !**/*.png, !figures/**
  • paper/manuscript.qmd is excluded by !**/*.qmd
📒 Files selected for processing (8)
  • reports/gate2_p3_ece.json
  • reports/p3/gate2_figure_data.json
  • reports/p3/stage2_scores_loo.json
  • src/tbox_finder/calib/gate2.py
  • tests/fixtures/eval_gate_sample/expected.json
  • tests/ml/test_eval_gate.py
  • tests/unit/test_gate2.py
  • workflow/rules/calibration.smk

Comment thread reports/gate2_p3_ece.json
Comment thread src/tbox_finder/calib/gate2.py
Comment thread src/tbox_finder/calib/gate2.py
Comment thread tests/ml/test_eval_gate.py
Comment thread tests/unit/test_gate2.py Outdated
…, label/test gaps

Both reviewers ran on 5cba66a: the CodeRabbit CLI (3 findings) and — unsolicited —
the GitHub app (5). Only ONE finding was common to both, so running both was
additive. All real, all addressed. GATE-2's numbers are unchanged: ECE 0.005662
<= 0.05, CI [0.002700, 0.009410], overall_pass true, report re-validates clean.

(both, MAJOR) The committed report carried this machine's filesystem layout.
Four fields held absolute paths including the OS user name and a
`.claude/worktrees/p3-10-gate2-ece` checkout, in a PUBLIC repo, beside
repo-relative siblings — so the artifact was also un-diffable across machines: a
re-run elsewhere changes a `provenance.inputs` KEY without any content changing.
Root cause is running from the main checkout with absolute paths into a linked
worktree. Every recorded path now routes through P3-08's
`stage2.eval.repo_relative`, which already relativises against both roots; the
repo has three forks of that helper and this adds no fourth. `provenance.inputs`
keys are normalised AFTER hashing, because `build_provenance` must be handed a
path it can actually open. The report was REGENERATED, not hand-edited.

(app, MAJOR) Diverting an invalid report was not enough on a RE-run. The
canonical path kept a previously accepted report — older numbers, older
provenance — reading as current beside a fresh `.invalid.json`, and the figure
data was written unconditionally to the path `plot_gate2_figures` consumes, so
the two artifacts could disagree. Both now go through one `write_outputs`, divert
together, and UNLINK the canonical file so its absence is the signal. Snakemake
deletes a failed job's outputs and had been masking this entirely.

A sabotage of that fix DID NOT BITE, and the gap was real: reverting `main`'s
call site left the suite green because the test covered `_output_path` but never
that `main` used it. The write is now one function, so the PAIRING is the unit
under test.

(app, minor) The reliability legend claimed one bin's count for every bin
(`rows[0]["n"]` + "rows each"); equal-MASS bins are only equal when n divides by
the bin count, which 3,045 happens to do. `_bin_size_label` reports a range
unless every bin really matches.

(app, minor) The eval-gate golden's three detection vectors were never asserted
aligned — `ood_ece`'s own length check cannot see a misalignment because `y` and
`block_labels` are both derived from one `keep`. Asserted where they meet. And
`n_excluded_no_block == 53` decomposes as 50 + 3 only while no sampled
dinucleotide source is one of the 3 null-`order` records — a property of the
decoy SEED, now asserted explicitly so a seed change fails readably.

(app, minor) `pytest.importorskip("numpy")` sat below the `tbox_finder` imports
it guarded, so collection would fail first and the guard was inert. Removed
rather than moved: the module's numpy dependency is hard.

(CLI, minor) RECORDED, NOT ACTED ON. 4 of 704 leave-clade-out decoys carry
positive logits (0.57% FPR at threshold 0 — verified at indices 8763/8809/9153/
9332), with a suggested negative-control FPR criterion. Also true: 864 of 8,641
held-out positives (10.0%) sit at non-positive logits — the leave-clade-out
RECALL story, GATE-1/P4 territory. A new FPR gate is a blinded-frozen default
requiring ADR sign-off (CLAUDE.md §7 item 2), so it is recorded rather than
invented inside a calibration step.

Validation: 56 test_gate2 + the eval-gate tier; full unit+golden+ml 3,902 passed
/ 14 failed — the 14 exactly a matched baseline worktree's set (comm empty both
ways). 52 sabotages, each RED against its NAMED test, each restored
byte-identically. ruff + black + snakemake --lint clean.
…, two logic bugs

CodeRabbit CLI, 6 findings (1 major, 5 minor), all real, all fixed. GATE-2's
numbers are unchanged across every regeneration: ECE 0.005662 <= 0.05, CI
[0.002700, 0.009410], overall_pass true, report re-validates clean.

⚠ The GitHub app did NOT review this round and its check went GREEN anyway —
`CodeRabbit  pass` carrying only "Review rate limited", which CLAUDE.md §5.1
classes as an absence. The CLI carried the gate alone. Across both rounds the app
reviewed once and the CLI twice.

(major) The figure projection dropped the not-gated markers. gate2_figure_data.json
held `gate: 0.05` and `in_distribution.passes` beside the OOD ECEs but none of the
guards the source report attaches to those same numbers, so a script reading only
that file could draw the in-distribution threshold across a panel where
Actinobacteria's mean is 0.68 and render a REPORTED quantity as a gate failure.

(minor, real logic bug) `--max-units` recorded a truncation that did not happen:
`--max-units 60` on a 30-unit holdout turned `graded_every_loo_holdout_unit` FALSE
while every unit had in fact been graded — a completeness clause failing on a
complete run. Now recorded only when the list was really cut.

(minor, real inconsistency) `_bin_concentration` compared `(value, index)` tuples,
resolving ties toward the LARGEST index, while its test asserted `.index(max(...))`
— the FIRST. A tie at the maximum is reachable exactly here: 11 of the 15 bins
carry a debiased gap of precisely 0.0. Selection is by value now, with a dedicated
tie fixture.

(minor) A replicate shortfall had no recorded reason, and chasing it explained the
mechanism exactly. Desulfuromonadales returned 181/200 and Pseudonocardiales
193/200. The kernel leaves out by ROW, so a replicate is undefined only when the
draw came entirely from a SINGLETON block. Measured block sizes: [1, 1, 31] and
[1, 2, 49]. One singleton in three blocks gives 1/27 = 3.7% -> 7.4 of 200, and
Pseudonocardiales lost exactly 7. Each unit now carries n_boot_requested,
n_boot_dropped and the reason.

A sabotage of that fix DID NOT BITE — the second time this session — and the gap
was real: the fixture had no singleton block, so it never dropped a replicate and
`requested == survived` made the sabotage invisible. The test now forces a
singleton (10 of 40 dropped) and asserts the shortfall is non-zero BEFORE
asserting the accounting closes.

(minor) The two schema_version fields version different schemas ("1" for the report
body, "1.0" for the provenance envelope); the scopes are now declared in the
artifact rather than left to be inferred. (minor) A contradictory legend comment
removed.

⚠ The hand-built OOD test fixture had drifted. A shape check against the shipped
`grade_ood_units` immediately found SIX keys it lacked — why_not_gated, by_phylum,
adjudicable_fraction, n_boot, bootstrap_seed, n_units_sub_min_n — every one a field
no test resting on that fixture could have been covering. The fixture now IS the
producer's output, so it cannot drift; the shape check was then dropped as a
tautology.

Adds a `figure-data` subcommand that re-derives the projection from a committed
report in seconds instead of re-running the 40-minute leave-one-out bootstrap; a
test pins it byte-identical to what `grade` writes.

Validation: 62 test_gate2 + the eval-gate tier; full unit+golden+ml 3,908 passed /
14 failed — the 14 exactly a matched baseline worktree's set (comm empty both
ways). 58 sabotages, each RED against its NAMED test, each restored
byte-identically. ruff + black + snakemake --lint clean.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/tbox_finder/calib/gate2.py (2)

700-708: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Attach n_boot_drop_reason only when replicates were dropped.

The code sets n_boot_drop_reason on every unit. In reports/gate2_p3_ece.json, 28 of the 30 units record n_boot_dropped: 0 and still carry the full explanation (for example lines 489-491 and 540-542). A reader of a single unit sees a reason for a drop that did not happen, and the committed artifact repeats the same 300-character string 30 times. Only Desulfuromonadales (line 1000) and Pseudonocardiales (line 1612) dropped replicates.

The test at tests/unit/test_gate2.py line 1129 only checks the reason on the unit that actually loses replicates, so it stays green.

♻️ Proposed change
         out["n_boot_requested"] = int(n_boot)
         survived = (out.get("ci") or {}).get("n_boot")
         out["n_boot_dropped"] = None if survived is None else max(0, int(n_boot) - int(survived))
-        out["n_boot_drop_reason"] = (
-            "replicates whose leave-one-out statistic was non-finite were dropped by "
-            "eval.resample.block_bootstrap: the kernel leaves out by row, so a replicate "
-            "drawn entirely from a singleton block leaves no row with a distinct-uid "
-            "neighbour. Rejected resamples, not a truncated bootstrap."
-        )
+        out["n_boot_drop_reason"] = (
+            (
+                "replicates whose leave-one-out statistic was non-finite were dropped by "
+                "eval.resample.block_bootstrap: the kernel leaves out by row, so a replicate "
+                "drawn entirely from a singleton block leaves no row with a distinct-uid "
+                "neighbour. Rejected resamples, not a truncated bootstrap."
+            )
+            if out["n_boot_dropped"]
+            else None
+        )
🤖 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/calib/gate2.py` around lines 700 - 708, Set
n_boot_drop_reason in the output-building logic only when n_boot_dropped is
greater than zero; omit the key or leave it unset when no replicates were
dropped. Preserve the existing explanation text for units that actually lose
replicates, and update any generated report or assertions affected by this
conditional behavior.

1687-1687: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

The key remap can silently drop a hash.

_recorded_path maps a path outside the repository onto a shorter locator. If two inputs ever normalize to the same string, this dict comprehension keeps only the last hash, and the provenance record loses one input without any signal. The three current inputs are all inside the repository, so reports/gate2_p3_ece.json lines 2038-2040 show three distinct keys today. Guard the invariant so a future input outside the checkout cannot collapse the record.

♻️ Proposed change
-    prov["inputs"] = {_recorded_path(k): v for k, v in prov["inputs"].items()}
+    recorded_inputs = {_recorded_path(k): v for k, v in prov["inputs"].items()}
+    if len(recorded_inputs) != len(prov["inputs"]):
+        raise ValueError(
+            "two inputs normalised to the same recorded locator, so one sha256 would be "
+            f"dropped from provenance: {sorted(prov['inputs'])}"
+        )
+    prov["inputs"] = recorded_inputs
🤖 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/calib/gate2.py` at line 1687, Update the provenance input
remapping around _recorded_path so normalized paths cannot overwrite an existing
hash: detect duplicate remapped keys and fail explicitly with a clear error
instead of silently retaining the last value. Preserve the existing remapped
dictionary for unique keys.
🤖 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/calib/gate2.py`:
- Around line 1577-1588: Update the figure-data handler to validate the source
report before writing, using the existing diversion behavior from write_outputs:
clean reports may write to the requested output, while rejected reports must not
publish at the canonical consumer path and must remove any stale canonical
artifact. Extend the equality test in test_gate2.py with a rejected-report case
asserting the canonical figure-data path remains absent.

---

Nitpick comments:
In `@src/tbox_finder/calib/gate2.py`:
- Around line 700-708: Set n_boot_drop_reason in the output-building logic only
when n_boot_dropped is greater than zero; omit the key or leave it unset when no
replicates were dropped. Preserve the existing explanation text for units that
actually lose replicates, and update any generated report or assertions affected
by this conditional behavior.
- Line 1687: Update the provenance input remapping around _recorded_path so
normalized paths cannot overwrite an existing hash: detect duplicate remapped
keys and fail explicitly with a clear error instead of silently retaining the
last value. Preserve the existing remapped dictionary for unique keys.
🪄 Autofix

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: 270a9ac0-fe99-4129-8b33-b7e9bed00759

📥 Commits

Reviewing files that changed from the base of the PR and between 5cba66a and ae6ea40.

⛔ Files ignored due to path filters (1)
  • analyses/phase3_log.qmd is excluded by !**/*.qmd
📒 Files selected for processing (5)
  • reports/gate2_p3_ece.json
  • reports/p3/gate2_figure_data.json
  • src/tbox_finder/calib/gate2.py
  • tests/ml/test_eval_gate.py
  • tests/unit/test_gate2.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/ml/test_eval_gate.py
  • reports/p3/gate2_figure_data.json

Comment on lines +1577 to +1588
if args.command == "figure-data":
# `figure_data` is a pure projection of the report, so re-deriving it from a
# committed report is byte-identical to what `grade` wrote — and costs seconds
# instead of re-running the leave-one-out bootstrap. A test pins that equality.
source = json.loads(Path(args.report).read_text(encoding="utf-8"))
out = Path(args.out)
out.parent.mkdir(parents=True, exist_ok=True)
out.write_text(
json.dumps(figure_data(source), indent=2, sort_keys=True) + "\n", encoding="utf-8"
)
print(f"wrote {out} (re-derived from {_recorded_path(args.report)})")
return 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

The figure-data subcommand bypasses the diversion invariant.

write_outputs refuses to place figure data for a rejected report at the consumer path, and it removes a stale canonical artifact. The figure-data handler does neither. It reads any report and writes the projection to --out, which defaults to DEFAULT_FIGURE_DATA.

A run such as figure-data --report reports/gate2_p3_ece.invalid.json therefore publishes figure data derived from a rejected grade at exactly the path plot_figures consumes. That is the outcome the paired writer added in this PR exists to prevent, and it reaches the canonical path without any validation.

Validate the source report and divert the output when it is not clean.

🐛 Proposed fix
     if args.command == "figure-data":
         # `figure_data` is a pure projection of the report, so re-deriving it from a
         # committed report is byte-identical to what `grade` wrote — and costs seconds
         # instead of re-running the leave-one-out bootstrap. A test pins that equality.
         source = json.loads(Path(args.report).read_text(encoding="utf-8"))
-        out = Path(args.out)
+        problems = validate_report(source)
+        # A projection of a REJECTED report must not reach the path `plot_figures` reads —
+        # the same invariant `write_outputs` enforces on the `grade` path.
+        out = _output_path(args.out, valid=not problems)
         out.parent.mkdir(parents=True, exist_ok=True)
         out.write_text(
             json.dumps(figure_data(source), indent=2, sort_keys=True) + "\n", encoding="utf-8"
         )
+        if problems:
+            Path(args.out).unlink(missing_ok=True)
         print(f"wrote {out} (re-derived from {_recorded_path(args.report)})")
-        return 0
+        for problem in problems:
+            print(f"  REPORT PROBLEM: {problem}")
+        return 1 if problems else 0

If you take this fix, extend the equality test at tests/unit/test_gate2.py lines 1143-1156 with a rejected-report case that asserts the canonical figure-data path stays absent.

📝 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
if args.command == "figure-data":
# `figure_data` is a pure projection of the report, so re-deriving it from a
# committed report is byte-identical to what `grade` wrote — and costs seconds
# instead of re-running the leave-one-out bootstrap. A test pins that equality.
source = json.loads(Path(args.report).read_text(encoding="utf-8"))
out = Path(args.out)
out.parent.mkdir(parents=True, exist_ok=True)
out.write_text(
json.dumps(figure_data(source), indent=2, sort_keys=True) + "\n", encoding="utf-8"
)
print(f"wrote {out} (re-derived from {_recorded_path(args.report)})")
return 0
if args.command == "figure-data":
# `figure_data` is a pure projection of the report, so re-deriving it from a
# committed report is byte-identical to what `grade` wrote — and costs seconds
# instead of re-running the leave-one-out bootstrap. A test pins that equality.
source = json.loads(Path(args.report).read_text(encoding="utf-8"))
problems = validate_report(source)
# A projection of a REJECTED report must not reach the path `plot_figures` reads —
# the same invariant `write_outputs` enforces on the `grade` path.
out = _output_path(args.out, valid=not problems)
out.parent.mkdir(parents=True, exist_ok=True)
out.write_text(
json.dumps(figure_data(source), indent=2, sort_keys=True) + "\n", encoding="utf-8"
)
if problems:
Path(args.out).unlink(missing_ok=True)
print(f"wrote {out} (re-derived from {_recorded_path(args.report)})")
for problem in problems:
print(f" REPORT PROBLEM: {problem}")
return 1 if problems else 0
🧰 Tools
🪛 ast-grep (0.45.0)

[info] 1584-1584: use jsonify instead of json.dumps for JSON output
Context: json.dumps(figure_data(source), indent=2, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.

(use-jsonify)

🤖 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/calib/gate2.py` around lines 1577 - 1588, Update the
figure-data handler to validate the source report before writing, using the
existing diversion behavior from write_outputs: clean reports may write to the
requested output, while rejected reports must not publish at the canonical
consumer path and must remove any stale canonical artifact. Extend the equality
test in test_gate2.py with a rejected-report case asserting the canonical
figure-data path remains absent.

…on, un-vacuum a test

CodeRabbit CLI, 3 findings (all minor), all real, all fixed. None touched the
report, so no regeneration: GATE-2 still reads ECE 0.005662 <= 0.05 and the
committed artifact is byte-unchanged.

The divert was one-directional. Round 2 made a rejected run unlink the canonical
artifact so its absence is the signal; the mirror was unhandled, so a rejected run
followed by an accepted one left `gate2_p3_ece.invalid.json` beside the fresh
accepted report — a rejected grade with older numbers next to an accepted one,
the same ambiguity the first unlink exists to prevent. `write_outputs` now clears
whichever of the pair it is not writing.

The figure subtitle formatted optional fields without the guards used two lines
below it. `figure_data` derives ece, ece_plugin and temperature through `.get(...)`
chains, so all three can be None — and a run that could not fit a temperature is
exactly the one whose figure someone wants to look at, where `f"{None:.4f}"` turns
a diagnostic into a crash. The CI guard was also asymmetric: it tested `lower` and
then formatted both ends. `_fmt` returns "n/a" for anything non-finite or
non-numeric, and both ends are checked.

Verified by hand under the `viz` env as a CLAUDE.md §8.5 manual gate — matplotlib
is in neither the local pytest env nor CI's pinned install list, so an
`importorskip` would skip everywhere and prove nothing. `plot_figures` renders
both PNGs, rc 0, on a figure-data with no ECE, no temperature, no concentration
block and a half-missing CI. The portable tier tests `_fmt`'s contract directly.

A loop-only test could grade nothing. `test_the_ood_bootstrap_blocks_are_split_
columns_never_record_ids` put its whole check inside
`for column in RS.RECORD_LEVEL_COLUMNS`, so an empty allowlist would make it pass
while checking nothing — item 1 of that module's own docstring, in the module that
names it. Both allowlists are now asserted non-empty, and the sabotage that empties
`RECORD_LEVEL_COLUMNS` turns it red.

Validation: 64 test_gate2; full unit+golden+ml 3,910 passed / 14 failed — the 14
exactly a matched baseline worktree's set (comm empty both ways). 61 sabotages,
each RED against its NAMED test, each restored byte-identically. ruff + black +
snakemake --lint clean.
…concentration

CodeRabbit CLI, 1 minor finding, fixed. Documentation only: no code path touched,
no artifact regenerated, no reported result moved.

`_bin_concentration`'s docstring said "14 bins had nothing to get wrong" and its
tie-break comment said "11 of 15 bins carry a debiased gap of exactly 0.0". The
committed report records n_bins_with_zero_debiased_gap: 12 and
n_bins_with_saturated_accuracy: 12, so both were wrong.

Re-derived from the report's own reliability table rather than re-read: 12 bins
(indices 1 and 4-14) sit at accuracy exactly 0 or 1 with a zero debiased gap, and
three bins carry a non-zero gap — index 0 (0.004812), index 2 (0.009547) and the
transition bin 3 (0.070565). 14 was the count of everything-but-the-transition-bin;
11 was an undercount that missed the single all-negative bin. The tie-break
rationale is unaffected: 12 identical values still make a tie at the maximum
reachable, which is why selection is by value with `.index()`.

paper/manuscript.qmd and analyses/phase3_log.qmd already said "12 of the 15",
which is correct, so no reported number changed. Every bin claim in the prose was
then re-derived against the artifact: n_bins 15, saturated 12, zero-gap 12,
top-bin share 0.8309, widest span 0.9956.

This is the fourth review round and the first carrying nothing behavioural.

Validation: 64 test_gate2; full unit+golden+ml 3,910 passed / 14 failed — the 14
exactly a matched baseline worktree's set (comm empty both ways). ruff + black
clean.
…librated unit

CodeRabbit CLI, 1 minor finding, fixed. Figure-rendering only: no report change,
no regeneration; the committed artifact is byte-unchanged since round 2.

`plot_figures` selected each unit's plotted value with
`(u["ood_ece"] if admissible else u["inadmissible_point"]) or 0.0` for the sort key
and `... or float("nan")` for the value. An OOD ECE of exactly 0.0 is falsy, and a
perfectly calibrated held-out order is a real outcome — its marker would vanish
from the panel, its error bar become NaN, and its sort key collapse onto that of a
genuinely missing value. Now tested against None.

A sabotage of the fix did not bite — the third time this session — and the gap was
the same one twice: the selector lived in a closure inside `plot_figures` and the
test re-implemented it locally, so inverting the shipped logic left the suite
green. The selector is now the module-level `ood_point`, and the test calls it
rather than a copy. A test that re-implements what it checks verifies the
re-implementation.

Render re-verified by hand under the viz env (CLAUDE.md §8.5 manual gate —
matplotlib is in neither the local pytest env nor CI's pinned list): both PNGs,
rc 0, on a figure-data carrying a 0.0-ECE unit with no CI.

Validation: 65 test_gate2; full unit+golden+ml 3,911 passed / 14 failed — the 14
exactly a matched baseline worktree's set. 62 sabotages, each RED against its
NAMED test. ruff + black + snakemake --lint clean.
…g different things

CodeRabbit CLI, 1 minor finding, fixed. The gated number is unchanged for the
sixth time: ECE 0.005662 <= 0.05, report re-validates clean.

Both readings of the census were correct, and that was the problem. The scores
file records block_census.n_singleton_blocks: 0 while 3,424 of its 4,332 blocks
contain exactly one row. Under P3-08's own definition 0 is right — that field
counts rows with NO CLUSTER at all (n_rows_without_cluster, also 0: every
leave-clade-out row is clustered). Under the natural reading of the name it is
wrong by 3,424.

The collision matters here specifically. Round 2 established that a bootstrap
replicate is undefined exactly when drawn entirely from a ONE-ROW block, and
recorded n_boot_dropped 19 and 7 on that basis. A reader cross-referencing the two
artifacts sees '0 singleton blocks' beside a non-zero drop whose stated cause is
singleton blocks, and reasonably concludes one of them is wrong.

Fixed where it bites, not where it originated: renaming P3-08's field would touch
a committed report schema and its shape-lock test for a naming issue. Each unit
now carries n_blocks_of_size_one — the count that governs its own drop — beside
n_boot_dropped, and the drop reason names the field it is NOT.

The regenerated report confirms the mechanism across the whole range:
Pseudonocardiales 1 one-row block of 3 -> 1/27 = 3.7% -> 7 dropped of 200;
Desulfuromonadales 2 of 3 -> 2/27 = 7.4% -> 19; Lactobacillales 2,366 one-row
blocks but 2,948 total -> 0. The ratio governs, not the count — which is why a
bare repo-wide singleton total explains nothing on its own.

Validation: 65 test_gate2; full unit+golden+ml 3,911 passed / 14 failed — the 14
exactly a matched baseline worktree's set. 63 sabotages, each RED against its
NAMED test. ruff + black + snakemake --lint clean.
…nute run

CodeRabbit CLI, 1 minor finding, fixed. No artifact touched, no regeneration.

`if target != "cpu": torch.cuda.empty_cache()` — `--device` accepts any string
torch accepts, so `mps` (or `xpu`) is reachable, and on a build without CUDA that
call raises. The raise lands AFTER score_rows and BEFORE the payload is written,
so the entire ~28 minute leave-clade-out scoring run would be lost to a cache
hint. Now gated on `torch.cuda.is_available() and str(target).startswith("cuda")`,
factored into `_should_release_cuda` so the predicate is testable without a GPU.

This is the same shape as the round-1 provenance defect: an incidental call at the
tail of an expensive function, positioned so its failure discards everything the
function just produced. Two of the three most costly defects this step found were
of that kind, and neither is visible from reading the happy path.

The same pattern exists at stage2/eval.py:1809 (P3-08). Not changed here — another
step's module with a committed artifact and a shape-locked report — but recorded
in the dev-log so it is not rediscovered as new.

Validation: 66 test_gate2; full unit+golden+ml 3,912 passed / 14 failed — the 14
exactly a matched baseline worktree's set. 64 sabotages, each RED against its
NAMED test. ruff + black clean.
@bioedca
bioedca merged commit 2ceca74 into main Aug 5, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants