Skip to content

Adds NW build scripts. - #1935

Open
danamouk wants to merge 1 commit into
MIT-LCP:mainfrom
danamouk:nw-build
Open

Adds NW build scripts.#1935
danamouk wants to merge 1 commit into
MIT-LCP:mainfrom
danamouk:nw-build

Conversation

@danamouk

Copy link
Copy Markdown
Contributor

This pull request adds PostgreSQL build scripts and documentation for the Northwestern ICU (NWICU) dataset on PhysioNet.

The following files are added under nw/buildnw/postgres/:

README.md – setup and usage instructions

create.sql – schema creation script

load_gz.sql – load compressed CSV data

constraint.sql – primary/foreign key constraints

index.sql – indexes for performance

validate.sql – validation checks

postgres_env.sh – environment variable helper

@Chessing234

Copy link
Copy Markdown
Contributor

nw build scripts — docs on required env vars / dump layout would save people a round trip.

@Chessing234 Chessing234 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.

two things, one small and one worth changing.

small: the first quickstart block has -f nw/buildnw postgres/load_gz.sql (missing slash) — it's correct in the longer walkthrough further down, but that first one is the copy-pasteable version and it fails.

the one worth changing: postgres_env.sh documents source postgres_env.sh myuser mypassword nw localhost 5432. that puts a database password into shell history and into the process arguments of anything watching. these are PhysioNet-credentialled users, so it's a real credential. read -s -p 'password: ' inside the script, or pointing people at ~/.pgpass, avoids it without costing convenience. the defaults are also worth a thought — sourcing with no arguments exports PGPASSWORD=your_password, which fails with a confusing auth error rather than prompting.

i could not check the expected row counts in validate.sql; i don't have NWICU access, so someone who does should confirm those.

@Chessing234 Chessing234 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.

went through these properly against mimic-iv/buildmimic/postgres/, which is what they're modelled on. most of it tracks line for line; three places diverge in ways i think are bugs rather than deliberate adaptation.

prescriptions_pk drops drug_type. mimic-iv has:

ADD CONSTRAINT prescriptions_pk PRIMARY KEY (pharmacy_id, drug_type, drug);

this branch has PRIMARY KEY (pharmacy_id, drug). drug_type is in the nw prescriptions table (VARCHAR(100)) and it's in the key upstream for a reason — a single pharmacy order routinely carries a MAIN row and a BASE/ADDITIVE row naming the same drug. if that pattern exists in NWICU at all, constraint.sql aborts after load_gz.sql has already inserted 1.85M prescription rows, and the operator has to work out what happened from a bare unique-violation. i can't check whether NWICU has such rows (no access), but there's no upside to narrowing the key relative to the source it was copied from — i'd restore drug_type.

d_items_pk is widened to (itemid, label), and that costs two foreign keys. mimic-iv uses PRIMARY KEY (itemid). adding label means the constraint no longer guarantees itemid is unique, which is the one thing anything joining d_items actually relies on — including the chartevents_to_* and procedureevents_to_* maps in #1930, which key on itemid alone.

it also explains a gap i'd otherwise have read as an oversight: mimic-iv declares chartevents_d_items_fk and procedureevents_d_items_fk, and neither exists here. they can't — a single-column FK on itemid has nothing to reference once the PK is a pair. so the widened PK and the two missing FKs are the same decision. if there really are duplicate itemids in nw_icu.d_items that's worth stating explicitly in the readme, because it's a meaningful difference from MIMIC; if there aren't, narrowing the PK back to itemid gets both FKs back.

the readme's quickstart only sets ON_ERROR_STOP=1 on the load step. mimic-iv's readme sets it on load, constraint and index:

psql -d mimiciv -v ON_ERROR_STOP=1 ... -f constraint.sql
psql -d mimiciv -v ON_ERROR_STOP=1 ... -f index.sql

here create.sql, constraint.sql, index.sql and validate.sql all run without it, so psql prints the error, keeps going, and exits 0. combined with the first point that's the difference between "the build failed on the prescriptions key" and "the build looked fine and the key silently isn't there". this one's a one-line fix and worth doing regardless of the other two.

smaller things:

  • constraint.sql adds diagnoses_icd_patients_fk and diagnoses_icd_admissions_fk twice — once in the primary-key section (lines 34/40) and again in the foreign-key section (112/118). the DROP CONSTRAINT IF EXISTS in front makes it harmless, but it reads as an editing leftover.
  • create.sql makes nw_hosp and nw_icu but no nw_derived, where mimic-iv's makes mimiciv_derived alongside the two. #2000 adds nw/concepts_postgres/, so something is going to need that schema.
  • 28 CREATE INDEX here against mimic-iv's 43. mostly that's fewer tables, but labevents gains an itemid index that mimic doesn't have while chartevents is (charttime, itemid) where mimic is (charttime, storetime). if those are deliberate tuning choices for NWICU's size, a comment saying so would stop the next person "fixing" them back.
  • validate.sql joins expected to observed with INNER JOIN, same as mimic-iv, so a table missing from either side silently drops out of the report instead of failing. inherited rather than introduced, but a build script whose failure mode is a shorter result set is worth an ORDER BY sanity count at least.

my two earlier notes still stand: the -f nw/buildnw postgres/load_gz.sql typo in the first quickstart block (missing slash), and postgres_env.sh taking the password as a positional argument.

as before, i have no NWICU access, so the expected row counts in validate.sql and whether the data actually violates the prescriptions key both need someone who does.

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