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.

[kevzho]

I can take over the remaining E13 review fixes and the real-manifest k-fold run: provenance hashes, the remaining test import cleanup, fold prevalence preflight, and per-fold evaluation. Since the original author is no longer assigned, please assign me or confirm I should push the follow-up changes.

[duckyquang]

Re-reviewed at 9310fc0, run locally against the real canonical manifest. @kevzho — thanks for picking this up; the provenance block and the prevalence preflight both do what #78 asked and my round-2 block is properly closed. But the committed fold metadata didn't come from the canonical manifest, so it needs regenerating before this merges.

Blocking

• data/processed/kfold/kfold_manifest.json was generated from a different manifest than the canonical one. It records input_sha256 cfb7664f… with input_path ../PRIMED-AI/data/processed/echo_hubert_manifest.parquet, 1,184 rows / 992 subjects. The canonical manifest — per this PR's own docs/results/SHA256SUMS line, and the file on disk — is 81694c9b…, 1,208 rows / 1,003 subjects. I reran make_kfold_splits.py on the real parquet: folds come out 702/100/201 subjects with val EF<=40 counts [26, 23, 23, 21, 29], against the committed [27, 27, 28, 21, 18], and every per-split subject hash differs. So the committed folds describe a cohort we don't publish results on — the exact drift #74 spent a week closing. Regenerate from the canonical parquet and recommit. The ../PRIMED-AI/ prefix suggests it was run from a sibling checkout, so run it from the repo root too, and the recorded input_path comes out repo-relative.

• Wrong home for the artifact. kfold_manifest.json is the only tracked file under data/, and it matches .gitignore:29 (/data/) — it's in the tree only because it was force-added. Every other committed artifact lives in docs/results/ with a SHA256SUMS line, and make_splits.py's equivalent output (cohort/splits.json) isn't committed at all. @sebasmos asked for the metadata to be committed and I agree, but it should land as docs/results/kfold_manifest.json with a SHA256SUMS entry rather than force-added past the ignore. Contents are safe to publish either way — split_subject_id_hashes hashes the whole ID list per split, not per subject, and no subject CSVs are committed.

• The runner never checks that kfold_manifest.json describes the parquets in --folds-dir. check_validation_prevalence reads val EF counts from the JSON while train_fold trains on fold_*.parquet, with nothing tying the two together — so stale metadata preflights clean against folds it has nothing to do with, and the new provenance block then hashes whatever is on disk without confirming it's the right thing. The mismatch above makes that live rather than theoretical. Cheap close: record each fold parquet's manifest_sha256 alongside manifest_path in make_kfold_splits.py, and compare it in load_kfold_metadata before training. Worth doing here specifically because fold generation is bit-reproducible — I ran it twice and got byte-identical parquets and identical metadata — unlike the checkpoints, which docs/results/README.md already documents as non-reproducible across machines. So that hash actually certifies something.

Checked & fine

• All three round-2 items are fixed. summary["provenance"] carries kfold_manifest_sha256 plus per-fold manifest_sha256 / fused_checkpoint_sha256, which is #78's provenance criterion. The EF<=40 counts are read now instead of write-only, via --min-val-positives (default 10) with --allow-low-val-positives to override. And the sys.path block is gone from test_kfold_cv.py (@sebasmos's item). The new tests pin all of it, including that a fold at 9 positives aborts before training.
• Suite at 9310fc0: 142 passed, 1 skipped. ruff check and ruff format --check both clean.
• Pooled out-of-fold is genuinely out-of-fold. evaluate_missing_modality.py writes test-split predictions under predictions and keeps val separately under predictions_val, so aggregate_results pools test rows only, and each subject lands in exactly one fold's test set.
• On the real folds the preflight has headroom — lowest val EF<=40 is 21 against a floor of 10 — but it isn't decorative: the committed (wrong) metadata dips to 18, so the per-fold spread is real and worth watching on every regeneration.

Nits

• check_validation_prevalence reports through warnings.warn, which is easy to lose in a long training log. print(..., file=sys.stderr) is louder for a CLI.
• fold_seed = args.seed immediately before the call is a no-op indirection — just pass args.seed. Reusing one seed across folds is the right call; worth a one-line comment saying it's deliberate.

@kevzho — yes from me on you taking E13 over, and I'll assign you. Ping me once the metadata is regenerated from the canonical parquet and I'll re-review. The per-fold training run on the real folds is still the remaining piece before this comes out of draft.

CHANGES REQUESTED