Skip to content

Commit 397feb5

Browse files
shaypal5claude
andcommitted
PR 6.2 Copilot review pass: fold all four real findings back in
Copilot's review of the PR 6.2 self-review surfaced four still-real findings (two others were marked outdated — already addressed by the self-review pass). Folded all four back in before requesting human review. - (COPILOT-1) Grammar: "reflect a honest production model" → "reflect an honest production model" in NB04 §2 narrative. - (COPILOT-3) The threshold-selection comment claimed the K-th highest probability + ``mask = lr_probs >= threshold`` admits *exactly* K leads, "ties resolved by score order". The inclusive comparison can admit more than K when leads share the threshold's probability — and there is no implicit tie-break. Rewrote the comment to be honest about the semantics ("admits AT LEAST K via probs >= threshold; ties at the threshold can inflate the slate; ``actually_above`` makes the realised count visible"). Kept the threshold-based selection rather than switching to a true top-K via ``argsort`` because the pedagogical point of section 6 is *threshold selection*, not *rank cutoff*. - (COPILOT-4) Bootstrap loop comment said "Degenerate resample — re-roll" but the implementation writes NaN and continues. Rewrote the comment to match what the code does (mark NaN, let ``_summary`` filter it out) and added the actual probability bound — with n_test=750 and base rate ~22 %, the all-positive or all-negative draw probability is ~10⁻¹⁰⁰, so the branch is dead in practice and exists only as a defensive safety net for tiny test sets. Implementing a real re-roll loop would never execute on this dataset. - (COPILOT-6) The top-level ``_doc`` in ``release/notebooks/_release_targets.json`` claimed the file contains only "cross-seed-median metric values", which became inaccurate after the PR 6.2 cohort_shift block (single-seed, seed 42). Rewrote the docstring to call out the mixed structure: per-tier blocks hold cross-seed medians; cohort_shift block holds single-seed values from ``validation_report.cohort_shift``. Audit-sync test continues to enforce both cases via separate loops. - (COPILOT-2, COPILOT-5) outdated; already addressed by the self-review fix-up commit (25b9ec1) — the 70/30-vs-85/15 narrative whiplash and the wrong summary numbers (0.19 / 2.6× vs the actual 0.13 / 2.75×) are both fixed there. Resolved as already-treated. Net: 28/28 notebook builder + execution + audit-sync tests pass; ruff + mypy clean; both notebooks still execute end-to-end in <10 s each. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 25b9ec1 commit 397feb5

3 files changed

Lines changed: 33 additions & 7 deletions

File tree

release/notebooks/04_lift_calibration_value_ranking.ipynb

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@
6363
"cell_type": "markdown",
6464
"id": "cell_003",
6565
"metadata": {},
66-
"source": "## 2. Train the headline LR + GBM panel\n\nSame preprocessing as notebooks 01 / 02 (mirrors\n`leadforge.validation.release_quality._build_pipeline`).\nWe drop the documented leakage trap `total_touches_all`\nhere so the calibration / lift / value plots in sections\n3–6 reflect a honest production model. The cohort-shift\nsection in section 7 uses the validator's full-panel\nposture (trap kept) so its number is comparable to the\npublished validation report."
66+
"source": "## 2. Train the headline LR + GBM panel\n\nSame preprocessing as notebooks 01 / 02 (mirrors\n`leadforge.validation.release_quality._build_pipeline`).\nWe drop the documented leakage trap `total_touches_all`\nhere so the calibration / lift / value plots in sections\n3–6 reflect an honest production model. The cohort-shift\nsection in section 7 uses the validator's full-panel\nposture (trap kept) so its number is comparable to the\npublished validation report."
6767
},
6868
{
6969
"cell_type": "code",
@@ -333,7 +333,14 @@
333333
"\n",
334334
"sorted_probs = np.sort(lr_probs)[::-1]\n",
335335
"# The K-th highest probability is the smallest threshold that\n",
336-
"# admits exactly K leads (ties resolved by score order).\n",
336+
"# admits AT LEAST K leads via ``probs >= threshold``. If\n",
337+
"# several leads share that probability, the inclusive\n",
338+
"# comparison can admit more than K — that's a property of\n",
339+
"# threshold-based selection, not a bug. The\n",
340+
"# ``actually_above`` readout below makes the realised count\n",
341+
"# visible so the operator can see when ties are inflating\n",
342+
"# the slate (and decide whether to break them with a\n",
343+
"# secondary score).\n",
337344
"threshold = float(sorted_probs[CAPACITY - 1])\n",
338345
"mask = lr_probs >= threshold\n",
339346
"n_above = int(mask.sum())\n",
@@ -508,7 +515,13 @@
508515
"for i in range(N_BOOT):\n",
509516
" idx = rng.integers(0, n_test, n_test)\n",
510517
" if y_test[idx].sum() == 0 or y_test[idx].sum() == n_test:\n",
511-
" # Degenerate resample — re-roll.\n",
518+
" # Degenerate resample (all-positive or all-negative)\n",
519+
" # — ``roc_auc_score`` is undefined here. We mark\n",
520+
" # the iteration NaN and let ``_summary`` filter it\n",
521+
" # out; with n_test=750 and base rate ~22 %, the\n",
522+
" # probability of a degenerate draw is ~10⁻¹⁰⁰, so\n",
523+
" # this branch is dead in practice. Kept as a\n",
524+
" # defensive safety net for tiny test sets.\n",
512525
" boot_lr_auc[i] = np.nan\n",
513526
" boot_gbm_auc[i] = np.nan\n",
514527
" boot_lr_ap[i] = np.nan\n",

release/notebooks/_release_targets.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"_doc": "Cross-seed-median metric values from release/validation/validation_report.json, sliced to the metrics the release notebooks pin via assert_within_tolerance. Audited against the report by tests/release/notebooks/test_release_targets_match_report.py — if you change a value here, the test will fail unless the corresponding median in the validation report changes to match.",
2+
"_doc": "Reproduction targets that release notebooks pin via assert_within_tolerance, sourced from release/validation/validation_report.json. Mixed structure: per-tier blocks (intermediate, ...) hold cross-seed-median metrics from validation_report.tiers.<tier>.medians; the cohort_shift block holds single-seed (seed 42) metrics from validation_report.cohort_shift.<tier>, since the report runs cohort-shift on seed 42 only. Audited against the report by tests/release/notebooks/test_release_targets_match_report.py — if you change a value here, the test will fail unless the corresponding source value in the validation report changes to match.",
33
"cohort_shift": {
44
"_doc": "Per-tier cohort-shift metrics from validation_report.cohort_shift (single-seed values; the report runs cohort-shift only on seed 42). Notebook 04 reproduces these via a chronological resplit and pins them via assert_within_tolerance.",
55
"intermediate": {

scripts/build_release_notebook_04.py

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ def cells() -> list[nbf.NotebookNode]:
128128
`leadforge.validation.release_quality._build_pipeline`).
129129
We drop the documented leakage trap `total_touches_all`
130130
here so the calibration / lift / value plots in sections
131-
3–6 reflect a honest production model. The cohort-shift
131+
3–6 reflect an honest production model. The cohort-shift
132132
section in section 7 uses the validator's full-panel
133133
posture (trap kept) so its number is comparable to the
134134
published validation report.
@@ -421,7 +421,14 @@ def acv_capture(use_value: bool, k: int) -> float:
421421
422422
sorted_probs = np.sort(lr_probs)[::-1]
423423
# The K-th highest probability is the smallest threshold that
424-
# admits exactly K leads (ties resolved by score order).
424+
# admits AT LEAST K leads via ``probs >= threshold``. If
425+
# several leads share that probability, the inclusive
426+
# comparison can admit more than K — that's a property of
427+
# threshold-based selection, not a bug. The
428+
# ``actually_above`` readout below makes the realised count
429+
# visible so the operator can see when ties are inflating
430+
# the slate (and decide whether to break them with a
431+
# secondary score).
425432
threshold = float(sorted_probs[CAPACITY - 1])
426433
mask = lr_probs >= threshold
427434
n_above = int(mask.sum())
@@ -644,7 +651,13 @@ def _gbm_pipeline_for_cohort() -> Pipeline:
644651
for i in range(N_BOOT):
645652
idx = rng.integers(0, n_test, n_test)
646653
if y_test[idx].sum() == 0 or y_test[idx].sum() == n_test:
647-
# Degenerate resample — re-roll.
654+
# Degenerate resample (all-positive or all-negative)
655+
# — ``roc_auc_score`` is undefined here. We mark
656+
# the iteration NaN and let ``_summary`` filter it
657+
# out; with n_test=750 and base rate ~22 %, the
658+
# probability of a degenerate draw is ~10⁻¹⁰⁰, so
659+
# this branch is dead in practice. Kept as a
660+
# defensive safety net for tiny test sets.
648661
boot_lr_auc[i] = np.nan
649662
boot_gbm_auc[i] = np.nan
650663
boot_lr_ap[i] = np.nan

0 commit comments

Comments
 (0)