fix: make unpurged feature-importance CV impossible to get silently - #27
Merged
Merged
Conversation
`_build_intervals(None, n)` yields degenerate one-row intervals `(i, i)`, which can only overlap the test rows themselves, so the purge step in `_purged_kfold_splits` removes zero additional training rows while the returned cv block still reports `method == "purged_kfold"`. Fails against current code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WYrS8fJ2Hi75CtuEy6AZVW
`_build_intervals(None, n)` built degenerate one-row intervals `(i, i)`. `_overlaps` reduces to `i == t` for two such intervals, so the purge loop only ever cleared rows that were already the test rows: the training set was every non-test row and `mda_importance`/`sfi_importance`/ `substitution_effect_report` returned leaked scores while reporting `cv["method"] == "purged_kfold"`. Measured on n=100, 5 folds, pct_embargo=0.0: the default trains fold 1 on all 80 non-test rows (zero purged); with real spans it trains on 60. `_build_intervals` now raises unless the caller passes label spans or opts out explicitly with `allow_unpurged=True`, and the `cv` block reports `purged` plus `kfold_embargo_only` so the opt-out cannot masquerade as purging. Deriving spans was rejected: an invented horizon replaces silent leakage with silent fabrication. BREAKING: calls that omitted `event_end_indices` now raise ValueError. That is the point -- they were returning leaked results. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WYrS8fJ2Hi75CtuEy6AZVW
The purging fix makes mda_importance/sfi_importance/substitution_effect_report raise when event_end_indices is omitted. Four documented call sites still called them without it, and would now raise: - moduleDocs.ts feature-diagnostics example (and its generated page) - workflows/python-core-workflow.md, which described the old buggy default - docs/python_bindings.md, which advertised purged defaults The examples now pass event_end_indices and explain why it is required, and show that cv.purged / cv.method make an explicit opt-out visible. Nothing automated caught this: check:examples covers Rust blocks only. A Python equivalent is tracked separately.
This was referenced Sep 18, 2026
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
1 participant
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.
event_end_indicesnow raise instead ofreturning leaked results. Fixed on the owner's instruction.
The bug
mda_importance(..., event_end_indices=None)purged nothing._build_intervals(None, n)produced(i, i)for every sample, so_overlapscollapsed toi == tand the purge loop only ever removed rows that were already in the test fold.
Measured, not theorised — n=100, 5 folds, embargo=0:
event_end_indices=None)So anyone calling it with defaults got silent leakage from the exact routine meant to prevent it,
and their results looked better than they were.
Found by an agent verifying the Python workflow by actually running it — not by reading it.
The fix
Raise unless the caller either passes spans or opts out explicitly with
allow_unpurged=True.The alternative — inventing a default horizon — was rejected on the grounds that it swaps silent
leakage for silent fabrication. Better to make the caller state their intent.
The result now reports
cv["method"](purged_kfoldvskfold_embargo_only) andcv["purged"],so an opt-out cannot masquerade as purging.
The error message explains the mechanism and the remedy rather than just failing.
Two sibling routines had the identical bug
sfi_importanceandsubstitution_effect_report— same shape, both fixed. The RustPurgedKFoldand
hyperparameter_tuningalready requiresamples_info_setsand were never affected.Verification
(3 ×
DID NOT RAISE), 5 passed after the fixcargo fmt --check0 · zero Rust changedastro build0 ·check-content-schema0 ·check-links0 ·check:api-drift0No existing test broke — because nothing was asserting the leaked behaviour.
Docs updated in the same PR
Four documented call sites would have started raising. They now pass
event_end_indicesand explainwhy it is required.
workflows/python-core-workflow.mdhad honestly documented the bug; it nowdocuments the fix.
Nothing automated caught that —
check:examplescompiles Rust blocks only. A Python equivalentis tracked separately, and this PR is the argument for it.