Skip to content

fix: Refresh artifact under corrected methodology, fix silently-ignor… - #7

Merged
Samarthuday merged 1 commit into
mainfrom
fix/temporal-validation-and-backtest
Aug 29, 2026
Merged

fix: Refresh artifact under corrected methodology, fix silently-ignor…#7
Samarthuday merged 1 commit into
mainfrom
fix/temporal-validation-and-backtest

Conversation

@Samarthuday

Copy link
Copy Markdown
Owner

…ed CSVs

Completes verification of the temporal-validation and walk-forward fixes against real data, per review checklist.

Real-data verification (all confirmed on the full 9.5M-row dataset):

  • Retrained via the corrected train_model.py: test-set metrics are byte-identical to before (PR-AUC 0.9859, ROC-AUC 0.9998) as expected -- the trained model/calibrator don't change, only where the threshold is selected from. Decision threshold moved from 0.001075 (old, selected from the calibration split) to 0.001160 (new, selected from the true held-out validation split). Calibration diagnostics are now honestly slightly worse (calibrated Brier 0.000078 vs 0.000043 previously) since they're now measured on data the calibrator never saw, rather than on its own fitting set.
  • Walk-forward backtest: 4 real windows spanning the full timeline, PR-AUC 0.938 -> 0.990 as the training window expands (previously: 0 windows, RuntimeError). Added to README as a new Temporal Stability table.
  • Unseen-entity evaluation re-run against the fresh artifact; added the fixed-production-threshold table to docs/research/unseen_entity_generalization.md -- one threshold (0.001160) applied to both subgroups lands each within noise of the intended 0.5% alert budget independently, recall is indistinguishable (99.3% vs 99.2%), precision differs 11x consistent with the subgroups' 12x prevalence gap.
  • python scripts/run_experiments.py --generalization run end-to-end against a real 284K-row sample of SAML-D (fast mode) to verify the full 6-stage orchestration and --features/--artifact passthrough work correctly; real committed reports/ were untouched (sample used isolated /tmp paths) and diffed byte-identical against docs/assets/ afterward.

Bug found while verifying the above: docs/assets/ablation_results.csv and docs/assets/typology_results.csv (and their reports/ counterparts) were never actually committed, despite prior commit messages claiming otherwise -- a blanket *.csv rule in .gitignore (meant for large raw datasets) was silently swallowing them since the first commit that tried to add them. This meant /api/results would return empty ablation/typology tables on any truly fresh clone, even though the PNG figures and JSON files (unaffected by the csv rule) worked fine -- the fallback-to-docs/assets resilience only half-worked. Fixed: !docs/assets/*.csv exception added, both files committed for the first time.

All 63 tests pass, ruff clean, API smoke-tested against the refreshed artifact (health/model-info/results all correct, decision_threshold and PR-AUC match the retrained artifact).

…ed CSVs

Completes verification of the temporal-validation and walk-forward fixes
against real data, per review checklist.

**Real-data verification (all confirmed on the full 9.5M-row dataset):**

- Retrained via the corrected train_model.py: test-set metrics are
  byte-identical to before (PR-AUC 0.9859, ROC-AUC 0.9998) as expected --
  the trained model/calibrator don't change, only where the threshold is
  selected from. Decision threshold moved from 0.001075 (old, selected from
  the calibration split) to 0.001160 (new, selected from the true held-out
  validation split). Calibration diagnostics are now honestly slightly
  worse (calibrated Brier 0.000078 vs 0.000043 previously) since they're
  now measured on data the calibrator never saw, rather than on its own
  fitting set.
- Walk-forward backtest: 4 real windows spanning the full timeline,
  PR-AUC 0.938 -> 0.990 as the training window expands (previously: 0
  windows, RuntimeError). Added to README as a new Temporal Stability table.
- Unseen-entity evaluation re-run against the fresh artifact; added the
  fixed-production-threshold table to docs/research/unseen_entity_generalization.md
  -- one threshold (0.001160) applied to both subgroups lands each within
  noise of the intended 0.5% alert budget independently, recall is
  indistinguishable (99.3% vs 99.2%), precision differs 11x consistent with
  the subgroups' 12x prevalence gap.
- `python scripts/run_experiments.py --generalization` run end-to-end
  against a real 284K-row sample of SAML-D (fast mode) to verify the full
  6-stage orchestration and --features/--artifact passthrough work
  correctly; real committed reports/ were untouched (sample used isolated
  /tmp paths) and diffed byte-identical against docs/assets/ afterward.

**Bug found while verifying the above:** docs/assets/ablation_results.csv
and docs/assets/typology_results.csv (and their reports/ counterparts) were
never actually committed, despite prior commit messages claiming otherwise
-- a blanket `*.csv` rule in .gitignore (meant for large raw datasets) was
silently swallowing them since the first commit that tried to add them.
This meant /api/results would return empty ablation/typology tables on any
truly fresh clone, even though the PNG figures and JSON files (unaffected
by the csv rule) worked fine -- the fallback-to-docs/assets resilience only
half-worked. Fixed: `!docs/assets/*.csv` exception added, both files
committed for the first time.

All 63 tests pass, ruff clean, API smoke-tested against the refreshed
artifact (health/model-info/results all correct, decision_threshold and
PR-AUC match the retrained artifact).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 29, 2026 17:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Samarthuday
Samarthuday merged commit bbe7921 into main Aug 29, 2026
2 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