E13: Add patient-level k-fold cross-validation

[slhhuang]

Implements the first part of #78 by adding reproducible patient-level k-fold split generation.

Current work:
5-fold patient-level splitting
Approximately 70/10/20 train/val/test per fold
Zero subject-level leakage checks
Every subject appears in exactly one outer test fold
Reproducible seeded splits
Unit tests and synthetic end-to-end validation

Validation completed:
103 tests passed
5 new k-fold tests passed
Ruff checks passed for the new files
Synthetic run successfully generated all five fold manifests and metadata

Still in progress:
Per-fold training of all four probes
Missing-modality evaluation for full, echo_dropped, and ecg_dropped
Across-fold and pooled MAE/AUROC
Final provenance updates after #74 is incorporated

Relates to #78

[duckyquang]

Ran everything locally in a clean worktree at adc56bf — the split logic does what it says and the leakage story is solid. One small fix needed before merge, and one question I'd like settled before part 2 starts training on these folds.

Blocking

• Fix ruff on tests/test_kfold_splits.py — with the repo config, ruff check fails (I001, un-sorted import block) and ruff format --check wants a reformat, so make lint / pre-commit will go red. Root cause is the manual sys.path.insert block at the top of the test: tests/conftest.py already puts scripts/ on the import path, so the whole block (and the # noqa: E402) can just be deleted and the import moved to the top like the other script tests (test_build_cohort.py etc.). I verified locally: with the block removed + ruff check --fix + ruff format, all 5 tests still pass and both ruff gates come back clean.

Question

• Should the fold metadata record per-fold EF<=40% counts? The canonical val split had only 26 positives, and each fold's val here is a fresh 10% draw, so some folds will dip lower — and part 2 selects checkpoints on val, which gets unstable fast at single-digit positives. #63 made check_ef40_prevalence.py a lock condition before the canonical split was trusted; same logic applies per fold. Cheap version: add EF<=40 counts per split to each entry in kfold_manifest.json so we see the problem before training, not after. Doesn't have to block this PR — but I'd want the counts visible before any per-fold checkpoint selection happens.

Checked & fine

• Full suite: 103 passed in the worktree, matching the PR description.
• Leakage: three independent checks (per-fold at construction, re-check on the output table, outer test-fold coverage), and the reproducibility tests pin same-seed/different-seed behavior.
• Proportions: with the real cohort's ~1,003 subjects, 5 folds land at ~201 test / 100 val / ~702 train — exactly the canonical 70/10/20 shape. The val_frac-relative-to-full-cohort choice is documented in a comment and does the right thing at n_folds=5.
• Fold manifests keep every row and reassign split, so each fold_*.parquet is drop-in for train_probes.py --manifest with zero downstream changes. Nice design — part 2 gets to reuse the whole existing training path.

Nits

• ~70 lines (file_sha256, hash_values, read_cohort, write_cohort, verify_no_subject_overlap) are copied verbatim from make_splits.py. from make_splits import ... works both as a script and under conftest, and keeps the leakage check single-sourced.
• The fold manifests silently overwrite the canonical split column. Intended, but consider preserving it as split_canonical (or adding a fold column) so a fold manifest can never be mistaken for the canonical one — provenance mixups are exactly what #74 spent a week cleaning up.
• No guard for val_frac >= 1 - 1/n_folds: --n-folds 2 --val-frac 0.5 silently produces an empty train set. Cheap ValueError.
• The synthetic end-to-end run you did by hand would make a good tmp_path test of main() — that would pin the CLI path (split overwrite, metadata write) too, not just the fold math.

CHANGES REQUESTED

[slhhuang]

Thanks so much for the detailed review. I’ve addressed the requested changes and the additional points you raised.

I removed the manual sys.path setup in test_kfold_splits.py, so Ruff and pre-commit are clean. I also added per-fold EF<=40 counts to kfold_manifest.json, preserved the original split as split_canonical, added a guard against val_frac values that would produce an empty training set, and added tests covering those behaviors. I also refactored make_kfold_splits.py to reuse the existing helper functions from make_splits.py instead of duplicating them.

The full suite now passes with 107 tests, and Ruff/format checks are clean.

Thanks again for catching these before the per-fold training stage.

[duckyquang]

Re-reviewed the whole PR including the new runner. All five round-1 items are properly fixed — verified locally at c44502d: 107 tests pass, ruff check and ruff format --check both clean this time, helpers imported from make_splits.py, split_canonical preserved and pinned by the new CLI test, the empty-train guard raises, and the EF<=40 counts use check_ef40_prevalence.normalize_ef_le_40 instead of reinventing it. Nice work, and thanks for turning it around fast.

I also went through run_kfold_cv.py against the real pipeline it shells out to, since a schema mismatch there would only surface after hours of training. It all checks out: train_probes.py accepts every flag you pass (--manifest --probe all --out-dir --epochs --fusion-dim --seed), the fused checkpoint really does land at {out-dir}/fused/cross_attn_fused.pt (_train routes by probe name), evaluate_missing_modality.py accepts --seed/--n-bootstrap/the dim flags, the payload keys you read (test[condition], bootstrap, predictions[condition] with lvef/prediction/ef_le_40) match what evaluation/missing_modality.py actually writes, and the pooled AUROC uses the same -prediction sign convention as the library. The across-fold mean±std vs per-fold bootstrap separation is exactly what #78 asked for, and testing aggregate_results with schema-matching fixtures plus a monkeypatched main was the right call.

One thing left before this merges:

Blocking

• kfold_results.json carries no provenance hashes — #78's acceptance criteria say "carry the manifest and per-fold checkpoint hashes in the run metadata, matching the provenance added in #74". Right now summary["config"] records paths and hyperparameters but not file_sha256 of each fold manifest or of each fold's fused checkpoint, so a results file can't be tied back to the folds/checkpoints that produced it — the exact drift #74 spent a week closing for the canonical artifacts. file_sha256 is already imported in make_kfold_splits.py; a per-fold {manifest_sha256, checkpoint_sha256} in the summary (plus the kfold_manifest.json hash once, to link back to the split generation) closes it.

Suggestion, not blocking

• The EF<=40 counts you added to kfold_manifest.json are currently write-only — run_kfold_cv.py never reads them. A cheap pre-flight in main() that loads the metadata and warns (or aborts without --force) when a fold's val positives are below some floor would spend that information before training time instead of after. Single-digit val positives is exactly where per-fold checkpoint selection goes unstable.

CHANGES REQUESTED

[sebasmos]

Ran the suite at c44502d: 107 passed, ruff clean; at 1,003 subjects the folds come out 702/100/201, matching the canonical split. Beyond the provenance block: generate the folds on the real manifest and commit kfold_manifest.json only (never the subject CSVs), and drop the sys.path block still in test_kfold_cv.py. REQUEST_CHANGES.