Repository navigation
Towards Unified Sequence Representation V: Add MIMIC-IV YAIB example for RNN_V2 - #1354
Conversation
Test CoverageTotal: 75.14% (21431 / 28522 lines) No modified files under |
200b2df to
4136b07
Compare
4136b07 to
e749b3e
Compare
e749b3e to
11ac43d
Compare
b78d7f1 to
e1e57f4
Compare
17a525d to
1ae0e3d
Compare
d6da9f9 to
17a31f3
Compare
examples/yaib_mimic4_unified_rnn.py trains a separate Unified_RNN on each of the five YAIB MIMIC-IV tasks (mortality, kidney function, length of stay, AKI and sepsis) and writes checkpoints, logs, the configuration and test metrics. --check verifies the data files and the device without preprocessing or training.
17a31f3 to
988b7c4
Compare
|
/review |
| } | ||
| REPO_ROOT = Path(__file__).resolve().parents[1] | ||
| TABLES = { | ||
| "hosp": ["patients", "admissions", "labevents", "transfers", "microbiologyevents"], |
There was a problem hiding this comment.
[medium] --check does not verify hosp/d_labitems.csv.gz, which preprocessing requires
The labevents config in pyhealth/datasets/configs/mimic4_ehr.yaml joins hosp/d_labitems.csv.gz on itemid with an inner join. BaseDataset loads that join file through _scan_table (pyhealth/datasets/base_dataset.py ~line 881), which raises FileNotFoundError when it is missing. preflight checks only the tables listed here, so --check reports success on a root that will then fail during preprocessing. The docstring says --check verifies the data files. Fix: add "d_labitems" to the "hosp" list. Also consider adding d_items for chartevents if that join is added to the config later.
| parser = argparse.ArgumentParser(description=__doc__) | ||
| parser.add_argument( | ||
| "--root", | ||
| default="D:/physionet.org/files/mimiciv/3.1", |
There was a problem hiding this comment.
[low] Default paths are one developer's Windows layout
The default --root (and --cache-dir at line 180, and the .venv/Scripts/python.exe invocation in the module docstring) hard-code D:/physionet.org/... and a Windows venv path. On other machines the script only fails later with a missing-file error. Consider making --root required, or documenting it in the docstring without a personal default.
There was a problem hiding this comment.
AI Review
suggestion: approve
decision: approve
Thank you for this contribution. The new example script trains a separate RNN_V2 on each of the five YAIB MIMIC-IV tasks. It includes a --check preflight and writes checkpoints, logs, config and test metrics. It only adds an example and changes no library code. The split is done before fitting the preprocessing statistics, which is the right order. It looks ready to merge.
Coverage: the PR changes no files under pyhealth/, so there is no per-file coverage to judge. Whole-repo coverage is 75.14%, which is above the 60% bar. Example scripts that need real MIMIC data are not normally unit-tested, so I don't see this as a problem.
Optional suggestions:
--checkis documented as verifying the data files, butTABLESdoesn't includehosp/d_labitems.csv.gz. The labevents config joins that file, so a machine missing it would pass--checkand then fail during preprocessing. Adding it to the hosp list would close the gap.- The default
--rootand--cache-dirand the docstring invocation (D:/...,.venv/Scripts/python.exe) assume one Windows layout. Platform-neutral defaults, or a required--root, and a plainpython examples/...invocation in the docstring would be easier for other users.
Reviewed commit 988b7c4
Fifth PR in the stack toward a unified sequence representation, on top of #1353. Example only; no library changes.
examples/yaib_mimic4_rnn_v2.pytrains a separateRNN_V2on each of the five YAIB MIMIC-IV tasks (mortality, kidney function, length of stay, AKI, sepsis). It writes checkpoints, logs, the configuration and test metrics.--checkverifies the data files and the device without preprocessing or training.This is a training example using PyHealth's YAIB task definitions, not a reproduction of the original benchmark's tuning and cross-validation protocol.