P2-13: Stage-1 temperature scaling + the non-gated reliability read (T = 0.9896) - #90
Merged
Merged
Conversation
…iability read (P2-13) The P2-13 machinery, plus the measured result that there is almost nothing to calibrate: T = 0.989587 on the production checkpoint (job 671 / P2-09 082f8c7), fitted on 1,778,688 per-window positions of the selection_val fit half, moving the per-position NLL 0.017839 -> 0.017836. Four things this step needs were unpinned - verified absent from PRD.md and all six ADRs: the calibration axis for a per-nt 8-class segmenter (D11 pins its estimator "on the positive-class posterior", written for Stage-2's binary head), single-T vs per-class T, the optimiser, and which fold (D11's "disjoint calibration split" occurs once in the PRD and its `calib` column is carved at P3-02, not here). ADR-0005 A11, signed 2026-07-30, pins all four. Inputs: data/processed/checkpoints/stage1_production/stage1.pt (DVC) P2-06a selection_val rung (830 records / 469 clusters) metrics.binned_ece (frozen: 15 equal-mass debiased bins, D11) Outputs: src/tbox_finder/calib/{__init__,temperature}.py metrics.reliability_bins (promoted out of binned_ece) reports/p2/calibration{,_figure_data}.json figures/calib/reliability_selection_val.png (git-LFS) tests/unit/test_temperature.py (66 checks, torch-free) docs/decisions/ADR-0005 A11 + the phase-2 dev-log stanza NON-GATED. GATE-2's ECE <= 0.05 is graded at P3 exit on the P3-02 carve; these numbers are on the fold the P2-06 sweep selected on, and no T here is shipped. T is fitted on one seeded whole-cluster half and read on the other, so the read is out-of-sample for T; the halves are validated by IDENTITY (the validator re-cuts the fit half from the recorded cluster->size map), because two ~equal halves make a swapped fit/read wiring invisible to every count in the artifact. Two findings beyond the headline: - a single shared T is the wrong instrument for what is miscalibrated here. The top bin of every rare class is over-confident (Stem I 0.599 predicted vs 0.570 observed), while background at 91.2% prevalence is under-confident - so the pooled NLL lands at T slightly BELOW 1, sharpening, the opposite of what the rare classes want. A measured argument for testing vector scaling at P3. - P2-12's flag is answered "no", and not by invariance: a single T preserves each WINDOW's arg-max, but the D3+A3 operator averages probabilities across overlapping windows, so it can move the RECONCILED arg-max wherever coverage >= 2 - which is 664,399 of 1,018,693 read positions. The mechanism is live; its magnitude at T=0.9896 is 2 positions. GATE-4 stays on the uncalibrated posterior. Two defects fixed on the way: the fit's |grad| <= tol stopping rule read an UNDERFLOWED gradient tail as stationarity and would have certified an arbitrary T on a perfectly-accurate fold (replaced by the exact beta->inf limit, which cannot underflow); and the first perfect-accuracy test matched either degeneracy message, so deleting the exact guard left the suite green - both guards now match their own message and were re-sabotaged separately. Validation: unit 66/66 (torch-free, runs in CI's no-torch env); eval-gate tier 13/13; the binned_ece refactor is bit-exact (130 old-vs-new comparisons, 0 differences; the committed expected.json ECE values reproduce to the last bit); four source-sabotage bites, each with its byte change shown and restored byte-identically; ruff@0.15.15 + black@25.11.0 clean; the artifact's own 7-clause gate re-derives from the report and passes. 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: 47 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 (2)
📝 WalkthroughWalkthroughAdds Stage-1 temperature-scaling calibration with deterministic cluster splitting, certified fitting, reliability metrics, report validation, CLI plotting, tests, and P2-13 JSON artifacts. ChangesTemperature calibration
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
…-reproducibility disclosure
Four CLI findings; three were real defects and one was a public-artifact path leak.
The GitHub CodeRabbit check reported only "Review rate limited" again, so the CLI is
what satisfied the CLAUDE.md 5.1 review gate.
(1) The fit could emit a report its own validator rejects. Bisection accepted
|grad| <= grad_tol OR a collapsed bracket as convergence; on the width path it
returned converged=True carrying a gradient larger than grad_tol - exactly the
clause validate_report checks (the P2-12 build_report-vs-validate_report shape,
one layer in). Only the stationarity residual certifies a fit now; a bracket that
collapses first raises. The committed artifact is unaffected: it converged on the
gradient (|grad| = 1.53 <= 1.78 at iteration 24, re-derivable from the recorded
bracket + n_iterations).
(2) validate_report could raise instead of report. Its contract is "total - returns
problems, never raises", but it did float(r.get("weight", 0.0)), r["debiased_gap"]
and per_class[e]["ece"] over unvalidated rows. A validator that crashes reports
nothing, which is strictly worse than one that reports a problem.
(3) A NaN or out-of-range posterior would have produced a plausible-looking ECE (the
estimator sorts by posterior, so a NaN sorts arbitrarily and returns a number).
The check lives in per_class_reliability, NOT in metrics.binned_ece: that kernel is
frozen and golden-locked, and a per-call scan of its Sequence inputs would run once
per class per bootstrap replicate (3,200 times here). One vectorised numpy check per
read covers every value this module hands it.
(4) The artifact recorded an absolute /home/<user>/... checkpoint path, unresolvable for
any reviewer of a public repo. Now relativised against the running checkout AND its
main checkout - this step runs from a .claude/worktrees/ worktree while the
DVC-tracked inputs are materialised only in the main checkout, so relativising
against the worktree alone would fall back to absolute for every real input.
The artifact is regenerated under the fixed code, and the re-runs measured something the
first run could not show: T = 0.9895867184568677 reproduces BIT-FOR-BIT across three
runs, as do every count (arg-max changes 2, coverage>=2 at 664,399, the cluster halves)
and gate4_core_min_f1 - but the artifact is NOT byte-identical. Per-class ECEs agree to
<= 3e-8 relative, the largest read disagreement is 4.5e-6 on a bin edge, and
grad_at_solution (a cancelling sum over ~1.8e6 terms) differs by 2.7e-4. The cause is the
GPU forward, not this module: the Mamba/CUDA kernels reduce in a run-dependent order. That
now ships as a ninth report disclosure carrying the measured bounds, so a reader re-derives
rather than diffing for equality.
Validation: unit 74/74 (8 new); four further source-sabotage bites, each with its byte
change shown and restored byte-identically (width-only convergence restored -> RED;
posterior-range check dropped -> RED; row shape-check dropped -> 3 RED; path
relativisation dropped -> RED); ruff@0.15.15 + black@25.11.0 clean; the artifact's own
7-clause gate re-derives and passes.
Claude-Session: https://claude.ai/code/session_01Tr67opmzHJAGBJPAb2aTdX
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/test_temperature.py (1)
521-525: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis substitution hides the gap it looks like it covers.
derive_clauses(junk if isinstance(junk, dict) else {})never passes the junk toderive_clauses, which is exactly where the totality claim breaks (unguardedreport.getinderive_clauses, see the comment onsrc/tbox_finder/calib/temperature.pylines 1018–1026). Once that is fixed, drop the substitution so the test bites.💚 Proposed tightening
for junk in (None, [], "report", 3): problems = T.validate_report(junk) # type: ignore[arg-type] assert problems and isinstance(problems, list) - assert T.derive_clauses(junk if isinstance(junk, dict) else {})["overall_pass"] is False + assert T.derive_clauses(junk)["overall_pass"] is False # type: ignore[arg-type]🤖 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_temperature.py` around lines 521 - 525, Update test_validator_is_total_on_junk to pass each junk value directly to T.derive_clauses instead of substituting {} for non-dictionaries, preserving the assertion that it returns overall_pass=False without raising.src/tbox_finder/calib/temperature.py (1)
689-694: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueHoist the per-cluster position grouping out of the class loop.
blocksis rebuilt by walking all read positions once per class (8 × N Python appends). The cluster → position-index grouping is class-independent, so compute it once and index the column inside the loop.♻️ Suggested restructure
+ pos_by_cluster: dict[int, list[int]] = {} + for pos, cid in enumerate(cluster_of_position): + pos_by_cluster.setdefault(cid, []).append(pos) for i, name in enumerate(CLASS_ORDER): - blocks: dict[int, list[tuple[int, float]]] = {} col = probs[:, i].tolist() - for pos, cid in enumerate(cluster_of_position): - blocks.setdefault(cid, []).append((1 if y_all[pos] == i else 0, col[pos])) + blocks = { + cid: [(1 if y_all[pos] == i else 0, col[pos]) for pos in idxs] + for cid, idxs in pos_by_cluster.items() + } per_class[name]["ece_ci"] = ece_block_ci(blocks, n_boot=n_boot, seed=seed)🤖 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/temperature.py` around lines 689 - 694, Move the cluster-to-position grouping work out of the CLASS_ORDER loop: build a reusable mapping of cluster IDs to position indices by iterating over cluster_of_position once, then in the per-class loop construct each class’s blocks from those indices using y_all and the current probability column. Keep the existing ece_block_ci call and n_boot/seed behavior unchanged.
🤖 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/temperature.py`:
- Around line 1176-1184: Update the figure-data write flow alongside the invalid
report handling: when problems is non-empty, divert the payload from
args.figure_data to its corresponding invalid artifact path or skip writing it,
preserving the existing canonical figure-data file. Keep valid runs writing to
args.figure_data as before, using the existing out invalidation logic in this
report-generation block.
- Around line 1018-1026: Update derive_clauses so operator_agrees_bit_exactly
and prior_shift_refused guard report with isinstance(report, Mapping) before
calling report.get, matching the existing guards and ensuring non-mapping inputs
such as None or lists return false clauses instead of raising AttributeError.
---
Nitpick comments:
In `@src/tbox_finder/calib/temperature.py`:
- Around line 689-694: Move the cluster-to-position grouping work out of the
CLASS_ORDER loop: build a reusable mapping of cluster IDs to position indices by
iterating over cluster_of_position once, then in the per-class loop construct
each class’s blocks from those indices using y_all and the current probability
column. Keep the existing ece_block_ci call and n_boot/seed behavior unchanged.
In `@tests/unit/test_temperature.py`:
- Around line 521-525: Update test_validator_is_total_on_junk to pass each junk
value directly to T.derive_clauses instead of substituting {} for
non-dictionaries, preserving the assertion that it returns overall_pass=False
without raising.
🪄 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: 2c291ee5-42c5-4351-9335-9dee1f5e963b
⛔ Files ignored due to path filters (3)
analyses/phase2_log.qmdis excluded by!**/*.qmddocs/decisions/ADR-0005-non-circular-eval-design.mdis excluded by!**/*.md,!docs/decisions/**figures/calib/reliability_selection_val.pngis excluded by!**/*.png,!figures/**
📒 Files selected for processing (6)
reports/p2/calibration.jsonreports/p2/calibration_figure_data.jsonsrc/tbox_finder/calib/__init__.pysrc/tbox_finder/calib/temperature.pysrc/tbox_finder/metrics.pytests/unit/test_temperature.py
Comment on lines
+1018
to
+1026
| "operator_agrees_bit_exactly": bool( | ||
| isinstance(report.get("operator_agreement"), Mapping) | ||
| and report["operator_agreement"].get("bit_exact") is True | ||
| ), | ||
| "not_gated": report.get("gated") is False if isinstance(report, Mapping) else False, | ||
| "prior_shift_refused": bool( | ||
| isinstance(report.get("stack_order"), Mapping) | ||
| and report["stack_order"].get("prior_shift_applied") is False | ||
| ), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
derive_clauses raises AttributeError on a non-mapping report.
Lines 1012–1013 and 1022 guard report with isinstance(..., Mapping), but the operator_agrees_bit_exactly and prior_shift_refused clauses call report.get(...) unguarded. derive_clauses(None) / derive_clauses([]) therefore crashes, while validate_report on the same input correctly returns problems — the "validator that dies reports nothing" failure this module explicitly designs against, one function over. Note that tests/unit/test_temperature.py:525 sidesteps it by substituting {} for non-dict junk, so the gap is not covered.
🛡️ Proposed fix
+ mapping = report if isinstance(report, Mapping) else {}
clauses = {
"report_valid": not problems,
"temperature_certified": bool(isinstance(temp, Mapping) and temp.get("converged") is True),
"read_out_of_sample_for_T": read_is_out_of_sample(report),
"operator_agrees_bit_exactly": bool(
- isinstance(report.get("operator_agreement"), Mapping)
- and report["operator_agreement"].get("bit_exact") is True
+ isinstance(mapping.get("operator_agreement"), Mapping)
+ and mapping["operator_agreement"].get("bit_exact") is True
),
- "not_gated": report.get("gated") is False if isinstance(report, Mapping) else False,
+ "not_gated": mapping.get("gated") is False,
"prior_shift_refused": bool(
- isinstance(report.get("stack_order"), Mapping)
- and report["stack_order"].get("prior_shift_applied") is False
+ isinstance(mapping.get("stack_order"), Mapping)
+ and mapping["stack_order"].get("prior_shift_applied") is False
),📝 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
| "operator_agrees_bit_exactly": bool( | |
| isinstance(report.get("operator_agreement"), Mapping) | |
| and report["operator_agreement"].get("bit_exact") is True | |
| ), | |
| "not_gated": report.get("gated") is False if isinstance(report, Mapping) else False, | |
| "prior_shift_refused": bool( | |
| isinstance(report.get("stack_order"), Mapping) | |
| and report["stack_order"].get("prior_shift_applied") is False | |
| ), | |
| mapping = report if isinstance(report, Mapping) else {} | |
| clauses = { | |
| "operator_agrees_bit_exactly": bool( | |
| isinstance(mapping.get("operator_agreement"), Mapping) | |
| and mapping["operator_agreement"].get("bit_exact") is True | |
| ), | |
| "not_gated": mapping.get("gated") is False, | |
| "prior_shift_refused": bool( | |
| isinstance(mapping.get("stack_order"), Mapping) | |
| and mapping["stack_order"].get("prior_shift_applied") is False | |
| ), |
🤖 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/temperature.py` around lines 1018 - 1026, Update
derive_clauses so operator_agrees_bit_exactly and prior_shift_refused guard
report with isinstance(report, Mapping) before calling report.get, matching the
existing guards and ensuring non-mapping inputs such as None or lists return
false clauses instead of raising AttributeError.
…nd de-vacuum the worktree path test
(1) An invalid report was diverted to calibration.invalid.json but its figure data kept
the canonical name - and the reliability diagram is rendered FROM that file, so a
refused run would still leave a plottable curve under the good name beside a report
marked invalid. Both artifacts now divert together (_output_paths), and the
figure-data build is guarded: diverting means execution CONTINUES past the write for
reports that previously never got there, so an unguarded figure_data(report) would
trade the validator's diagnostic for a KeyError traceback (the P2-12 round-4 shape,
reproduced by my own round-1 fix). The summary's numeric format specs get the same
treatment via _render -> "n/a".
(2) The worktree-aware path assertion was gated on "am I running inside a worktree" -
true on this laptop, FALSE in CI, which checks the repo out normally. It would have
certified the branch only where nobody looks. _portable_path now takes an injectable
checkout and the rule is asserted unconditionally on a synthetic tree, including the
two directions that must NOT fire (a sibling checkout stays absolute; a plain
checkout must not climb to its parent).
The committed artifact is untouched: the only behavioural changes are on the invalid path
and in stdout formatting, and _output_paths(..., valid=True) returns exactly the paths the
valid run already used.
Validation: unit 78/78 (4 new); three further source-sabotage bites, each byte-verified
and restored (figure data left at the canonical name -> RED; bare numeric format spec
restored -> RED; worktree->main-checkout root dropped -> RED); a post-refactor CLI smoke
(16 records, --n-boot 5) writes both artifacts and passes its gate; ruff/black clean.
Claude-Session: https://claude.ai/code/session_01Tr67opmzHJAGBJPAb2aTdX
… but never checked read.n_positions_coverage_ge_2 is reported, printed by the CLI summary, and load-bearing for the A11 Pin 4 reading (the coverage>=2 mass is what makes the reconciled arg-max movable at all), yet it was the only counter validate_report did not check. Now validated like the rest: non-int, boolean, or negative is a reported problem. The committed artifact is unaffected and this was CHECKED, not assumed: re-validating reports/p2/calibration.json under the new clause set returns zero problems and re-derives overall_pass=True (recorded value 664,399). A new gate clause can silently invalidate every already-committed report, so adding one means re-running the validator over the artifacts in the tree. Validation: unit 81/81 (3 new - missing, negative, and a boolean sneaking through isinstance(x, int)); one byte-verified sabotage bite (clause dropped -> both new cases RED, restored byte-identically); ruff/black clean. Claude-Session: https://claude.ai/code/session_01Tr67opmzHJAGBJPAb2aTdX
This was referenced Aug 3, 2026
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.
Fits the Stage-1 calibration machinery (imp.md P2-13) and reports the non-gated in-distribution reliability read. Nothing here is a calibration claim: GATE-2's ECE ≤ 0.05 is graded at P3 exit on the P3-02
calibcarve (PRD.md:56/:248, ADR-0005 D11), and noTfitted here is shipped.The §7 stop this step opened, and how it closed
Four things P2-13 needs are unpinned — verified absent from
PRD.mdand all six ADRs: the calibration axis for a per-nucleotide 8-class segmenter (D11 pins its estimator "on the positive-class posterior" — wording written for Stage-2's binary head), single-T vs per-class T, the optimiser, and which fold (D11's "disjoint calibration split" occurs exactly once in the PRD, and thecalibcolumn it names is carved at P3-02). Signed as ADR-0005 A11 (2026-07-30):metrics.binned_ece; worst-over-core, no cross-class meanβ = 1/T— no seed, no optimiser, no torch (so the unit gate runs in CI, which installs none)selection_val: fit on half A, read on half B, disclosed as not D11's splitResult
T = 0.989587 (±5.8e-5, re-derivable from the recorded bracket + iteration count), fitted on 1,778,688 per-window positions; per-position NLL 0.017839 → 0.017836. Per-class one-vs-rest ECE on the read half (1,018,693 nt / 176 clusters): background 0.0071, Stem I 0.0019, Stem II 0.0010, Stem III 0.0013, Terminator 0.0014, Antiterminator 0.0008, Specifier 0.0001, Discriminator 0.0002 — every value moves by ≤ 1.7e-5 under
T, in both directions.Two findings beyond the number:
Tslightly below 1, sharpening, the opposite of what the rare classes want. A measured argument for testing vector scaling at P3, which is exactly what A11 declined to pin without evidence.Tpreserves each window's arg-max, but the D3+A3 operator averages probabilities across overlapping windows, so it can move the reconciled arg-max wherever coverage ≥ 2 — 664,399 of 1,018,693 read positions. The mechanism is live; its magnitude at T = 0.9896 is 2 positions (0.0002 %). GATE-4 stays graded on the uncalibrated posterior.Defects fixed on the way
|grad| ≤ tolstopping rule read an underflowed gradient tail as stationarity, and would have certified an arbitraryTon a perfectly-accurate fold. Replaced by the exactβ → ∞limitΣ(max_c z − z_label), which cannot underflow.imp.md's P2-09 status marker still read☐ todofor a step merged as P2-09: the round-0 production Stage-1 checkpoint — job 671 on the full 8,303-record D5 fold, plus the Hydra-override gate that job 669 died for #61 — corrected (local-only file).Validation
binned_ece→reliability_binspromotion is bit-exact: 130 old-vs-new comparisons, 0 differences, and the committedtests/fixtures/eval_gate_sample/expected.jsonvalues reproduce to the last bitruff@0.15.15+black@25.11.0clean at CI-exact versionshttps://claude.ai/code/session_01Tr67opmzHJAGBJPAb2aTdX
Summary by CodeRabbit
New Features
Tests