P3-08: the with/without-aux ablation on the calibrated binary head + the Stage-2 score producer - #101
Conversation
… + the Stage-2 score producer (P3-08)
imp.md P3-08. PRD §11 (with/without-aux check), §12, §2.3 GATE-2;
ADR-0005 D11 (named posterior, 15 equal-mass debiased bins, ECE <= 0.05) +
D16 (aux weighting); ADR-0004 A7 (test is GATE-2's split, calib is where T is fit).
Inputs: data/processed/checkpoints/stage2_rinalmo/aux{1.0,0.0}_lr1e-4/
data/processed/stage2_dataset.parquet (calib 1,089 / val 3,067 / test 3,045)
calib/recalibrate.py (P3-07); metrics.binned_ece / average_precision /
block_bootstrap_ci
Outputs: src/tbox_finder/stage2/eval.py
reports/stage2_aux_ablation.json
reports/p3/stage2_scores.json (7,201 rows x 2 arms of per-row logits)
tests/unit/test_aux_ablation_check.py (62 pass, numpy-only)
tests/ml/test_stage2_eval_smoke.py (12 pass on the pinned ml-rna stack)
analyses/phase3_log.qmd stanza
This step had to build what it was specified to consume. Nothing in the repo read
a Stage-2 checkpoint back -- stage2_heads.pt and lora_adapter/ had write sites and
zero readers, and train.py::evaluate thresholds tbox_logit at 0 and keeps a scalar
accuracy -- so the per-row binary logits the calibration stack eats did not exist.
load_stage2_checkpoint + score_rows are that producer; P3-07's block already
assigned it here.
VALIDATION: gate.overall_pass = FALSE, and that is the measurement, not a shortfall.
arm calib wrong@0 T test ECE test AUPRC
aux1.0_lr1e-4 (prod) 1 / 1,089 1.140627 0.005662 0.999824
aux0.0_lr1e-4 (no-aux) 0 / 1,089 NONE undefined 0.999897
The no-aux arm's calib carve is PERFECTLY SEPARATED, so the exact minimiser is the
beta -> infinity limit and fit_temperature refuses it. The refusal is recorded as
structured evidence and `ece` is None -- never the T=1 value, because a substituted
temperature is indistinguishable from a measured one (CLAUDE.md §10.3). Two clauses
are false in consequence. The production arm holds GATE-2 at 8.8x under the D11 gate.
Median |logit| is 8.26 with-aux vs 14.45 no-aux: the aux terms shrink the head's
margins and are what keep it calibratable at all. The with-aux fit's entire signal
for beta is ONE misclassified calib row of 1,089 -- P3-10's GATE-2 grade currently
hangs on that record. Carried to P3-09/P3-10.
TWO §7 STOPS OPEN, neither resolved here:
1. imp.md grades "beyond a pre-registered tolerance"; no tau is pinned in PRD.md,
any ADR, conf/ or src/. D16 admits an absolute reading (needs no number; it
passes) and a delta reading (needs one). Both computed, neither resolved;
ablation.verdict = requires_signoff and the validator REFUSES a report that
pins a tolerance itself.
2. No ADR pins a degenerate-limit rule and there is no pre-registered fallback for
an uncalibratable arm.
Design notes: AUPRC is graded on logits, not the posterior -- z -> z/T is strictly
monotone so it cannot move average precision, but sigma() saturates to exactly 1.0
at these margins and would tie the ranking away. The adapter load is MEASURED: every
adapter and head tensor is compared bit-exactly against the file and non-zero lora_B
blocks are counted, because PEFT only warns on missing keys and initialises lora_B
to zero -- a silently-unloaded adapter is the untuned backbone producing a complete
and meaningless report.
Local: ruff 0.15.15 + black 25.11.0 clean; unit+golden+ml 3,750 passed, the 14
failures a strict subset of main's 31 in this runner (comm -13 empty => zero
regressions). 23 sabotages, each RED against its NAMED test, each restored
byte-identically.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 50 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 (3)
📝 WalkthroughWalkthroughAdded a Stage 2 evaluation pipeline for matched with-aux and no-aux arms. It performs strict checkpoint verification, deterministic scoring, temperature calibration, ECE/AUPRC grading, report validation, and committed ablation reporting. ChangesStage 2 auxiliary-ablation evaluation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Checkpoints
participant Dataset
participant Evaluator
participant Report
CLI->>Checkpoints: discover and verify matched arms
CLI->>Dataset: load rows and metadata
CLI->>Evaluator: score rows and grade arms
Evaluator->>Report: build and validate ablation report
Report-->>CLI: gate result and diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…o whose fixes were untested Review gate met by the CodeRabbit CLI (`coderabbit review --committed --base main`), not the GitHub app: its check went green carrying only a "Review rate limited" notice, which CLAUDE.md §5.1 classes as an absence. (`--plain` was removed in CLI 0.7.1 and now exits 1 with a usage dump; CLAUDE.md's invocation was stale.) MAJOR - reading_absolute was gated on the DELTA's availability. D16's absolute reading asks only whether the with-aux arm still holds the D11 grade, and that arm has an ECE. The run had a definite verdict all along: the absolute reading PASSES (0.005662 vs the 0.05 gate). Artifact regenerated. MAJOR - auprc_rank_invariant compared two floats for exact equality. `z/T` is strictly monotone in exact arithmetic but only non-decreasing in float64, so a rounding collision could flip a gate clause with no ranking loss behind it. The invariant is now stated directly (scaling created no ties), with the tie count and the AP delta recorded beside it. MINOR - the cluster-presence test was `isinstance(raw, float)`. np.float64 satisfies it and this column happens to be float64, but a float32 NaN would read as CLUSTERED, key `cluster:nan`, and collapse every unrelated decoy into one bootstrap block -- the failure block_keys exists to prevent, reintroduced by a type check. Normalised before the NaN test; a non-numeric id is refused by name. MINOR - the committed artifacts embedded absolute developer paths in a PUBLIC repo, publishing the OS user name and local layout permanently for paths that resolve on one machine. Now repo-relative. The first version of this fix BROKE THE RUN: relativising the path the loader OPENS is a different job from relativising the path the report RECORDS, and they are now separate fields (checkpoint_path/checkpoint_dir). MINOR - safetensors load_file can return mmap-backed tensors; the sabotage fixtures now clone before overwriting the file they were read from. TWO OF THE FIVE FIXES WERE UNTESTED, AND SABOTAGE IS WHAT SAID SO. Reverting the absolute-reading fix and reverting the tie-count fix each left all 70 tests green. The former needed a fixture where only the CONTROL lacks an ECE -- the shipped run's own configuration, which every existing fixture had on the other side. The latter is unreachable through grade_arm, so the check was extracted as `ranking_preserved` and tested directly on a FOUND float64 collision where scaling destroys ranking information while leaving average precision exactly unchanged. Reproducibility measured, not assumed: three independent scoring runs produced BIT-IDENTICAL logits for all 7,201 rows (max |delta| = 0). Validation: test_aux_ablation_check 72 pass; ml tier 12 pass; unit+golden+ml 3,758 passed, the 14 failures a strict subset of main's 31 (comm -13 empty). ruff + black clean. Artifacts carry zero absolute paths; validate_report clean. 6 further sabotages, each RED against its NAMED test, each restored byte-identically.
… an overclaiming docstring MINOR - scoring.batch_size recorded the flag THIS process was invoked with, even on the --scores-from path where the logits were produced under a different batching. Batch composition perturbs the bf16 reductions (which is why score_rows sorts deterministically), so the artifact could document a batching that never touched those numbers. Now takes the cached run's own value; the report also states regraded_from_cached_scores and names the sidecar. MINOR - validate_report's docstring claimed an empty list meant "well-formed AND passing". It does not: it checks that the recorded clauses match the re-derived ones and that overall_pass agrees. The shipped report is honestly failing and fully valid; conflating the two would invite someone to "fix" a truthful artifact. The batch-size fix was taken as an opportunity to make the shortcut's refusals reachable without a GPU: the --scores-from reconciliation is now `reconcile_cached_scores`, graded in the numpy tier. A mismatched dataset digest, a different row set at the SAME COUNT, and a missing arm are each refused by name. Re-grading a stale cache is the one failure this shortcut can introduce and the one that would look most like success. Validation: test_aux_ablation_check 78 pass; ml tier 12 pass; unit+golden+ml 3,766 passed, the 14 failures a strict subset of main's 31 (comm -13 empty). ruff + black clean. Artifact regenerated: 0 absolute paths, validate_report clean, headline numbers unchanged. 3 further sabotages, each RED against its named test, each restored byte-identically.
…es not exist, and a Linux-only leak guard MINOR - the module docstring named `ablation.verdict_delta_reading`; the report writes `ablation.reading_delta.verdict`. A pointer that does not grep is worse than none, because it reads as authoritative. MINOR - the end-to-end path-leak guard greped for "/home/", so an artifact regenerated on macOS (/Users/...) or Windows (C:\Users\...) would have passed while leaking exactly what the guard exists to catch. It now walks the JSON and asserts NO string value is absolute at all -- structural rather than a prefix list, since what makes a path a leak is that it is absolute, not which OS produced it. The detector carries its own non-vacuity test (planted paths of all three shapes must be found; a relative path must not), and the widening was verified by planting a /Users/... path in the real artifact: red under the new check, green under the old one. Validation: test_aux_ablation_check 79 pass; unit+golden+ml zero regressions; ruff + black clean; artifact regenerated and clean.
…the refusal classifier MINOR - `_classify_refusal` read `"no rows" in text or "zero" in text and "calib" in text`. Python binds `and` tighter than `or`, so the first disjunct stood alone and ANY "no rows" refusal was classified empty_calib_rung -- including one raised about a GRADED rung, which has nothing to do with calib. The branch name claims calib under both readings, so both readings must require it. Now parenthesised, with a table-driven test over all five refusal shapes plus a control, sabotage-verified: reverting the parentheses turns exactly the 'test'-rung case red. Validation: test_aux_ablation_check 85 pass; unit+golden+ml 3,773 passed, the 14 failures a strict subset of main's 31 (comm -13 empty). ruff + black clean.
… governs (user sign-off) Both §7 stops resolved by user sign-off (AskUserQuestion, 2026-08-03). DECISION 1 - the ABSOLUTE reading of ADR-0005 D16 governs. D16 asks whether the aux heads degrade "the calibrated primary head's GATE-2 grade", and that grade (D11) is a pass/fail: ECE <= 0.05. The check is whether the with-aux arm still HOLDS it -- which it does, at 0.005662, 8.8x under the gate. No tau is pinned and no ADR is amended. ablation.verdict = aux_does_not_degrade_the_gate2_grade The delta stays reported-not-gated: reading_delta.tolerance is still None and validate_report still REFUSES any report that pins one. Both readings are still computed and the tau-window on which they would disagree is still published, because a decision is only auditable beside the alternative it rejected. The verdict is DERIVED, never assigned: `verdict_from_absolute_reading` computes it from the governing reading's own evidence and `validate_report` re-derives it, so a report cannot carry a verdict its numbers contradict. DECISION 2 - accept and record the uncalibratable control now; pin the degenerate-limit rule in ADR-0005 D11 at the phase-exit gate, because P3-10 grades GATE-2 and will meet the same fact. gate.overall_pass stays false, and the report now states what that does NOT mean: not evidence that the aux heads degraded anything, but the recorded consequence of the CONTROL being uncalibratable. Enlarging or re-stratifying the calib carve until a misclassified row appears was rejected as choosing the measurement to obtain the answer. One test bug found while writing the validator test and worth naming: the "wrong" verdict was hard-coded, and on this synthetic fixture it was silently the RIGHT one. It is now derived as whichever verdict the report's own evidence does not support. Validation: test_aux_ablation_check 86 pass; ml tier 12 pass; unit+golden+ml 3,774 passed, the 14 failures a strict subset of main's 31 (comm -13 empty). ruff + black clean. Artifact regenerated: validate_report clean, 0 absolute paths, headline numbers unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/ml/test_stage2_eval_smoke.py (2)
318-334: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe test name promises a raw-logit check; the assertion only proves batching consistency.
Line 333 compares the batched score with a direct single-row forward pass. If a sigmoid were applied in
score_rowsand in the direct call, both sides would move together and this test would stay green. The docstring states the stronger property: the calibration stack must receive an unsquashed value.An additional assertion that ties the scored value to the head's own output range would name the property directly. A blunt
not 0.0 <= value <= 1.0check risks flaking on the tiny randomly-initialised backbone, so prefer a structural check, for example asserting thatscore_rowsreturns exactly the tensor the head emits for a deliberately large-magnitude input.🤖 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_stage2_eval_smoke.py` around lines 318 - 334, Add a structural raw-logit assertion to test_the_scored_logit_is_the_raw_head_output_not_a_probability: use a deliberately large-magnitude head output and verify score_rows preserves that exact unsquashed value, rather than only matching a direct forward pass. Avoid a generic [0, 1] range check, and retain the existing batching-consistency and token-count assertions.
250-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe comment claims an empty
base_model_name_or_pathis accepted, but no assertion tests it.Lines 250-251 state that a locally-constructed base records no name and must not be refused. The test then only sets
LH.REPO_ID. The falsy branch of the guard ineval.py(Line 1373,if recorded_base and recorded_base != LH.REPO_ID) stays uncovered, so a change that refuses an empty value would keep this file green.💚 Proposed addition
config["base_model_name_or_path"] = LH.REPO_ID config_path.write_text(json.dumps(config), encoding="utf-8") assert _load(ckpt, torch)[1]["n_adapter_tensors_matched"] > 0 + + # ...and the empty case the comment above promises: no recorded name is not a + # mismatch, it is the absence of a claim. + config["base_model_name_or_path"] = None + config_path.write_text(json.dumps(config), encoding="utf-8") + assert _load(ckpt, torch)[1]["base_model_name_or_path"] is 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 `@tests/ml/test_stage2_eval_smoke.py` around lines 250 - 256, Extend the positive-control test around config["base_model_name_or_path"] to explicitly set it to an empty value and assert _load still succeeds with matched adapter tensors. Keep the existing LH.REPO_ID assertion, ensuring both the pinned repository ID and empty recorded base paths are accepted.src/tbox_finder/stage2/eval.py (3)
901-917: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the refusal when a run report omits
aux_weightorlr.Lines 916-917 index
loss["aux_weight"]andconfig["lr"]directly. If a sweep report lacks either key, the caller sees a bareKeyError: 'aux_weight'with no arm name and no path. Every other refusal in this module names the file and the cause.♻️ Proposed change
+ for holder, key, where in ((loss, "aux_weight", "config.loss"), (config, "lr", "config")): + if key not in holder: + raise ValueError( + f"{report_path}: arm {arm_dir.name} records no {where}.{key}, so it " + "cannot be placed in the ablation" + ) arms[arm_dir.name] = {🤖 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/eval.py` around lines 901 - 917, Update the arm record construction around aux_weight and lr to validate that loss["aux_weight"] and config["lr"] are present before indexing them, and raise the module’s established error type with a message naming the arm and report path when either is missing. Preserve the existing float conversion and successful-record behavior.
1576-1592: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBuild each padded batch on the CPU, then move it once.
Lines 1580-1584 allocate
input_idsandattention_maskdirectly ontargetand then write one row at a time withtorch.tensor(ids, device=target). On a CUDA target that is two small host-to-device copies per row, plus a device-side slice assignment per row. For 7201 rows per arm the launch overhead adds up and buys nothing.Assembling the batch on the host and issuing one transfer per batch keeps the numerics identical.
⚡ Proposed change
- input_ids = torch.full((len(chunk), width), TOK.PAD_ID, dtype=torch.long, device=target) - attention_mask = torch.zeros((len(chunk), width), dtype=torch.long, device=target) - for i, (n, _, ids) in enumerate(chunk): - input_ids[i, :n] = torch.tensor(ids, dtype=torch.long, device=target) - attention_mask[i, :n] = 1 + input_ids = torch.full((len(chunk), width), TOK.PAD_ID, dtype=torch.long) + attention_mask = torch.zeros((len(chunk), width), dtype=torch.long) + for i, (n, _, ids) in enumerate(chunk): + input_ids[i, :n] = torch.tensor(ids, dtype=torch.long) + attention_mask[i, :n] = 1 + input_ids = input_ids.to(target) + attention_mask = attention_mask.to(target)🤖 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/eval.py` around lines 1576 - 1592, Update the batching logic in the inference loop to allocate `input_ids` and `attention_mask` on the CPU, populate all rows there, then transfer each completed tensor to `target` once per batch before calling `model`. Preserve the existing padding values, masks, tensor shapes, and logits/output behavior.
744-754: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider re-deriving
reading_absolute.passesfrom the recorded ECE.
passescopiesw["ece_gate_pass"]andvalidate_reportthen derives the verdict frompasses. Nothing comparesobserved_eceagainstgate. A hand-edited artifact that setsece: 0.2andece_gate_pass: truestill validates, because no check closes that loop. The module states elsewhere that clauses must be re-derived rather than believed.A cheap closure is to compute
passesfrom the number the report already carries.♻️ Proposed change in
compare_arms- absolute_passes = bool(w["ece_gate_pass"]) if with_aux_ece_available else None + # Derived from the recorded ECE, not read back from the arm's own pass flag, so a + # report cannot carry a `passes` its own number contradicts. + absolute_passes = bool(M.gate2_ece_pass(w["ece"])) if with_aux_ece_available else NoneA matching clause in
validate_report(or a new clause inderive_clauses) that assertsreading_absolute.passes == (observed_ece <= gate)would make the guard complete.🤖 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/eval.py` around lines 744 - 754, Update compare_arms so reading_absolute.passes is re-derived from the recorded observed_ece and ECE_GATE rather than copied from w["ece_gate_pass"], while preserving the unavailable-ECE handling. Add or update validation in validate_report or derive_clauses to assert that reading_absolute.passes matches the observed_ece <= gate comparison.tests/unit/test_aux_ablation_check.py (2)
750-827: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
CLAUSE_SABOTAGEcovers every derived clause.The list currently names all 15 clauses. Nothing fails when someone adds a clause to
derive_clausesand forgets to add its sabotage, so the new clause ships untested. One assertion closes that gap and keeps the sabotage suite honest as the clause set grows.💚 Proposed test
+def test_every_derived_clause_has_a_sabotage() -> None: + """A clause with no sabotage is a clause nothing proves can go false.""" + assert {name for name, _ in CLAUSE_SABOTAGE} == set(E.derive_clauses(_report()))🤖 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_aux_ablation_check.py` around lines 750 - 827, Update test_every_clause_bites to assert that the clause names in CLAUSE_SABOTAGE exactly match the keys returned by E.derive_clauses(_report()), including detecting missing or extra sabotage entries. Keep the existing per-clause sabotage assertions unchanged.
236-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe direct-fit reference encodes
recalibrate's binary-to-two-class layout.Lines 239-241 build the reference logits as
[0, z]and compare the resulting temperature tograde_arm's atrel=1e-9. That exact equality holds only whileR.temperature_scaleuses the same two-column embedding for a scalar binary logit. If that module changes to a symmetric layout such as[-z/2, +z/2], this test fails for a reason unrelated to the rung-selection property it asserts.A short comment naming the coupling, or asserting against
R.temperature_scaleon the test-rung slice instead ofTEMP.fit_temperature, keeps the failure message pointing at the real cause.🤖 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_aux_ablation_check.py` around lines 236 - 247, The direct-fit reference in the test around _arm and TEMP.fit_temperature is coupled to recalibrate’s binary-to-two-class [0, z] embedding. Document that coupling with a concise comment, or instead compute the expected temperature through R.temperature_scale on the test-rung slice so the assertion remains valid if the binary layout changes while still verifying rung selection.
🤖 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/stage2/eval.py`:
- Around line 1003-1012: Align the gate evaluation with the documented non-gated
--all-arms behavior: keep load/scoring clauses over all discovered arm_blocks,
but restrict calibration and grading clauses to the ablation pair by introducing
or reusing pair_blocks. Update the relevant calibs, stacks, graded_for_ece, and
graded clause calculations to read pair_blocks, while preserving the existing
all-arm reporting.
---
Nitpick comments:
In `@src/tbox_finder/stage2/eval.py`:
- Around line 901-917: Update the arm record construction around aux_weight and
lr to validate that loss["aux_weight"] and config["lr"] are present before
indexing them, and raise the module’s established error type with a message
naming the arm and report path when either is missing. Preserve the existing
float conversion and successful-record behavior.
- Around line 1576-1592: Update the batching logic in the inference loop to
allocate `input_ids` and `attention_mask` on the CPU, populate all rows there,
then transfer each completed tensor to `target` once per batch before calling
`model`. Preserve the existing padding values, masks, tensor shapes, and
logits/output behavior.
- Around line 744-754: Update compare_arms so reading_absolute.passes is
re-derived from the recorded observed_ece and ECE_GATE rather than copied from
w["ece_gate_pass"], while preserving the unavailable-ECE handling. Add or update
validation in validate_report or derive_clauses to assert that
reading_absolute.passes matches the observed_ece <= gate comparison.
In `@tests/ml/test_stage2_eval_smoke.py`:
- Around line 318-334: Add a structural raw-logit assertion to
test_the_scored_logit_is_the_raw_head_output_not_a_probability: use a
deliberately large-magnitude head output and verify score_rows preserves that
exact unsquashed value, rather than only matching a direct forward pass. Avoid a
generic [0, 1] range check, and retain the existing batching-consistency and
token-count assertions.
- Around line 250-256: Extend the positive-control test around
config["base_model_name_or_path"] to explicitly set it to an empty value and
assert _load still succeeds with matched adapter tensors. Keep the existing
LH.REPO_ID assertion, ensuring both the pinned repository ID and empty recorded
base paths are accepted.
In `@tests/unit/test_aux_ablation_check.py`:
- Around line 750-827: Update test_every_clause_bites to assert that the clause
names in CLAUSE_SABOTAGE exactly match the keys returned by
E.derive_clauses(_report()), including detecting missing or extra sabotage
entries. Keep the existing per-clause sabotage assertions unchanged.
- Around line 236-247: The direct-fit reference in the test around _arm and
TEMP.fit_temperature is coupled to recalibrate’s binary-to-two-class [0, z]
embedding. Document that coupling with a concise comment, or instead compute the
expected temperature through R.temperature_scale on the test-rung slice so the
assertion remains valid if the binary layout changes while still verifying rung
selection.
🪄 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: 996d4145-68ba-42cb-ac74-961543184ac8
⛔ Files ignored due to path filters (1)
analyses/phase3_log.qmdis excluded by!**/*.qmd
📒 Files selected for processing (5)
reports/p3/stage2_scores.jsonreports/stage2_aux_ablation.jsonsrc/tbox_finder/stage2/eval.pytests/ml/test_stage2_eval_smoke.pytests/unit/test_aux_ablation_check.py
| arms = report.get("arms") or {} | ||
| ablation = report.get("ablation") or {} | ||
| dataset = report.get("dataset") or {} | ||
| clauses: dict[str, bool] = {} | ||
|
|
||
| arm_blocks = [a for a in arms.values() if isinstance(a, Mapping)] | ||
|
|
||
| # -- the score producer actually produced scores from the trained weights ------- # | ||
| loads = [(a.get("load") or {}) for a in arm_blocks] | ||
| clauses["adapter_weights_verified_against_file"] = bool(loads) and all( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--all-arms is documented as non-gated, but every discovered arm is graded here.
arm_blocks covers every entry of report["arms"]. With --all-arms, that is every trained arm, and each one must satisfy adapter_weights_verified_against_file, temperature_positive_and_converged, graded_object_is_pre_prior_shift and the rest. One unrelated arm that cannot be temperature-calibrated then turns gate.overall_pass false while the ablation pair is clean.
The CLI help at Line 1707 states the opposite: "score every trained arm, not just the ablation pair (reported, non-gated)".
Either restrict the clause set to the ablation pair, or correct the help text. The load/scoring clauses arguably should still cover every scored arm; the calibration clauses arguably should not.
🔧 One way to restrict the calibration and grade clauses to the ablation pair
arm_blocks = [a for a in arms.values() if isinstance(a, Mapping)]
+ pair_names = [ablation.get("with_aux_arm"), ablation.get("no_aux_arm")]
+ pair_blocks = [a for n, a in arms.items() if n in pair_names and isinstance(a, Mapping)]and then read pair_blocks for calibs, stacks, graded_for_ece and graded.
🤖 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/eval.py` around lines 1003 - 1012, Align the gate
evaluation with the documented non-gated --all-arms behavior: keep load/scoring
clauses over all discovered arm_blocks, but restrict calibration and grading
clauses to the ablation pair by introducing or reusing pair_blocks. Update the
relevant calibs, stacks, graded_for_ece, and graded clause calculations to read
pair_blocks, while preserving the existing all-arm reporting.
…ero, and an unvalidated JSON boundary MINOR - separation_census returned 2 keys on an empty rung and 10 on a populated one, so a consumer reading is_perfectly_separated off the committed report hits a KeyError. The empty path IS reachable: grade_arm records calib_separation even when the fit refused BECAUSE the calib rung was empty. And `n_misclassified_at_zero: 0` on an unmeasured rung reads exactly like a measured result -- the same substitution this module refuses to make for `ece`. Both paths now return the same key set, with None for everything not measured. MINOR - ArmScores is constructed from external JSON by reconcile_cached_scores, with no shape or finiteness check. A nested list satisfies the length check (an (n, k) array has len n) and a NaN would flow all the way to the fit before anything objected, far from the file that carried it. Both are now checked where the data enters. Validation: test_aux_ablation_check 91 pass; ml tier 12 pass; unit+golden+ml 3,779 passed, the 14 failures a strict subset of main's 31 (comm -13 empty). ruff + black clean. 2 further sabotages, each RED against its named test, each restored byte-identically.
imp.md P3-08. PRD §11 (with/without-aux check), §12, §2.3 GATE-2; ADR-0005 D11 (named posterior; 15 equal-mass debiased bins; ECE ≤ 0.05) + D16 (aux weighting); ADR-0004 A7 (
testis GATE-2's split,calibis whereTis fit).The step had to build what it was specified to consume
imp.md scopes P3-08 as "LOCAL (analysis of already-trained arms)" — but the calibrated posteriors it analyses did not exist. Nothing in this repo read a Stage-2 checkpoint back:
stage2_heads.ptandlora_adapter/had write sites and zero readers, andtrain.py::evaluatethresholdstbox_logitat 0 (train.py:1161) and keeps only a scalar accuracy. Sostage2/eval.pyships the score producer (load_stage2_checkpoint,score_rows) alongside the ablation entry. P3-07's block already assigned the producer here.Result —
gate.overall_pass = false, and that is the measurementTaux1.0_lr1e-4(production)aux0.0_lr1e-4(no-aux)The no-aux arm cannot be calibrated at all. Its calib carve is perfectly separated, so the exact minimiser is the β→∞ (T→0) limit and P3-07's
fit_temperaturerefuses it. The refusal is recorded as structured evidence andeceisNone— never theT = 1value, because a substituted temperature is indistinguishable from a measured one (CLAUDE.md §10.3). Two clauses are false in consequence; nothing was invented to make them true.The production arm holds GATE-2 at 8.8× under the D11 gate. On the axis that survives, ΔAUPRC = −7.4e-5. Median |logit| 8.26 vs 14.45 is the mechanism: the aux terms shrink the head's margins and are what keep it calibratable.
⚠ The with-aux fit's entire signal for β is ONE misclassified calib row of 1,089. P3-10's GATE-2 grade currently hangs on that record. Carried to P3-09/P3-10.
Two §7 stops, neither resolved here
PRD.md, any ADR,conf/orsrc/. D16 admits an absolute reading (needs no new number — and it passes) and a delta reading (needs one). Both are computed, neither resolved:ablation.verdict = requires_signoff, andvalidate_reportrefuses a report that pins a tolerance itself.Design notes for review
z ↦ z/Tis strictly monotone so it cannot move average precision (asserted as a gate clause), butσ()saturates to exactly 1.0 at these margins and would tie the ranking away.PeftModel.from_pretrainedonly warns on missing keys and PEFT initialiseslora_Bto zero, so a silently-unloaded adapter is the untuned backbone producing a complete, plausible, meaningless report. Every adapter and head tensor is compared bit-exactly against the file, in both directions, and non-zerolora_Bblocks are counted.cluster_idbecomes a singleton bootstrap block, never one giant block.Validation
ruff0.15.15 +black25.11.0 clean. unit+golden+ml: 3,750 passed; the 14 failures are a strict subset ofmain's 31 in this local runner (comm -13empty ⇒ zero regressions; CI runs the pinned stack). 23 sabotages, each RED against its named test, each restored byte-identically. Scoring: 2 arms × 7,201 rows, 12 min 33 s on the laptop RTX 4060, forward-only.Summary by CodeRabbit
New Features
Tests