feat(#49): verify EHR resource loader against real MIMIC-IV data

[MohShahin]

What

Resolves the remaining ask on #49. The loader (load_resource_contexts)
and registry entry were already implemented and fixture-tested in PR #66;
what stayed open was explicitly the owner's step per @sebasmos's comment:
"verify on the real data... confirm the row-level parse." This PR does
that. Two rounds of review caught real methodology and schema problems
along the way -- both are documented below and fixed, not glossed over.

• Adds scripts/mimic_iv/derive_ehr_resource_contexts.sql, a documented
  BigQuery query deriving the scenario_id, icu_stay_count, budget_pressure
  CSV from icustays/admissions (runs directly against PhysioNet's
  hosted BigQuery tables).
• Adds a regression test for a real-world parsing case found while
  validating against actual data: a genuinely empty (not missing) value
  in a non-required column must round-trip as "" in meta.
• Adds a doc pointer from ehr.py's module docstring to the script.

Definition of done, checked directly

Confirmed against real MIMIC-IV data, not just synthetic fixtures:
94,382 real ICU-stay scenarios load into ResourceContext with zero
parse errors.

Schema, as actually shipped

ResourceContext has scenario_id, icu_stay_count, budget_pressure,
meta. There is no beds or staffing field.

• icu_stay_count: the number of distinct ICU stays recorded under the
  same hospital admission. This is a real, MIMIC-native count, not a
  fabricated number -- used as a resource-load proxy on the reasoning
  that an admission requiring multiple ICU stays/transfers plausibly
  indicates higher acuity, but it does not measure real bed occupancy or
  staffing ratios.
• budget_pressure: hospital length of stay in days. This is a
  fabricated proxy -- MIMIC-IV has no real cost tables. Flagged explicitly
  below.

Methodology history (two issues caught in review, both fixed)

1. Cross-patient timestamp comparability. MIMIC-IV shifts dates
independently per subject_id into the 2100-2200 range, so a naive
cross-patient intime/outtime overlap join (the first version of this
query) is statistically meaningless. My first attempted fix bucketed the
join on patients.anchor_year_group -- this was incorrect, caught by
@Agastya191 and @sebasmos: sharing an anchor_year_group does not make
two patients' independently-shifted timestamps comparable. The actual
fix derives everything within a single hadm_id, which needs no
cross-patient comparison at all and is immune to the shift problem by
construction.

2. Two column names for one algebraically-dependent number. After
fix 1, the query still emitted both beds (= icu_stay_count) and
staffing (= 1 / icu_stay_count) -- not two independent signals, one
number under two labels that both described things the data doesn't
measure (real occupancy, real nurse ratios). Caught by @Agastya191 and
@sebasmos. Fixed by shipping the single honestly-named icu_stay_count
column and dropping beds/staffing entirely, including from
REQUIRED_COLUMNS and the ResourceContext dataclass.

budget_pressure (hospital LOS) was unaffected by either issue above
-- always a within-patient, within-admission interval, which MIMIC-IV's
shift preserves exactly. A small fraction of admissions have
dischtime < admittime (documented MIMIC-IV data-entry artifact);
excluded via WHERE hosp_los_days >= 0.

Real-data validation, final numbers (current schema)

Check	Result
Rows loaded	94,382 / 94,382, zero parse errors
budget_pressure	zero negative rows
Empty-string values in meta	present (e.g. unset insurance) -- covered by the regression test

icu_stay_count distribution:

icu_stay_count	count	%
1	77,475	82.1%
2	13,068	13.8%
3	2,718	2.9%
4	704	0.7%
5	280	0.3%
6	90	0.1%
7	28	<0.1%
9	9	<0.1%
10	10	<0.1%

No real MIMIC-IV data is committed anywhere in this PR -- only the SQL
(code) and a synthetic fixture matching the real-world shape discovered.
Full export lives locally only, per MIMIC-IV's DUA.

Flagging for review -- not settled by this PR

budget_pressure remains a fabricated proxy. This is exactly the kind of
methodology choice #297 says needs sign-off. Flagging for @sebasmos
before it's treated as the real cascade shortcut feature for this lane.
icu_stay_count is a real signal but its use as a resource-constraint
proxy specifically (vs. e.g. an acuity proxy) is still a design choice
worth confirming.

Testing

tests/test_ehr_adapter.py: 10 cases (1 new: empty-string-in-meta), all
passing under the current schema. Full suite: 636 passed, 9 skipped, no
failures. ruff check clean.

Closes #49. Thanks to @Agastya191 and @sebasmos for catching both the
cross-patient timing issue and the two-names-one-number schema problem in
review -- the shipped version is meaningfully more honest for it.

[Agastya191]

good work on derive_ehr_resource_contexts.sql and on actually running the loader over a real export, the hosp_los_days >= 0 filter and the empty-string meta regression test are both genuine catches. One problem is that in derive_ehr_resource_contexts.sql, bucketing the concurrent_counts join by anchor_year_group does not make the intime/outtime overlap comparable: MIMIC-IV shifts dates independently per subject_id, so two patients in the same 3-year group still sit at arbitrary offsets from each other inside 2100-2200, and overlapping in shifted time says nothing about overlapping in real time. This means beds still counts coincidental shift collisions and staffing is exactly 1/beds, so the 102 to 37 drop is the candidate pool splitting five ways rather than the number becoming real, and both the script header and the new ehr.py docstring now tell readers the comparison is sound. You have multiple options to fix this, for example I would derive the load proxy from something within an admission such as ICU stay count or LOS per hadm_id, and leave beds and staffing out until there is a shift-invariant way to compute them.

[sebasmos]

Good real-data validation, and the empty-string and LOS fixes are solid catches. One issue on beds/staffing though: MIMIC-IV shifts dates per patient, so same anchor_year_group doesn't mean actually concurrent, and right now that's what's driving the numbers. Agastya's idea of using ICU-stay-count or LOS per admission instead sounds like the right fix, let's hold off on beds/staffing until that's sorted.

[Agastya191]

Thanks for retracting the anchor_year_group approach instead of patching around it. The header comment in derive_ehr_resource_contexts.sql spelling out why the 102-to-37 drop was the pool shrinking rather than the number getting real is exactly the right way to record a wrong turn, and deriving within a single hadm_id does genuinely sidestep the shift problem.

One problem is that the new proxies no longer match what the adapter says they are: the query now emits beds as c.icu_stay_count and staffing as ROUND(1.0 / c.icu_stay_count, 4), while SPEC.raw_hint and the ResourceContext docstring in ehr.py still describe bed occupancy and a nurse-to-patient ratio, and neither was updated in this commit. That leaves two of the three numeric constraint columns as algebraically one variable instead of two independent signals, and an admission with a single ICU stay lands on beds=1.0 and staffing=1.0, so the scrutiny panel reads a near-constant column where the resource-constraint signal is supposed to be, and a reader seeing beds=3 will take it as three beds rather than one patient moved through the ICU three times.

You have multiple options to fix this. For example, I would keep the within-admission count but name it for what it measures and put that name in REQUIRED_COLUMNS, leaving beds and staffing out until there is a source for real occupancy, rather than shipping two column names for one number under labels that mean something else. Smaller and separate: the PR description still presents the anchor_year_group bucketing as fix #1 alongside the beds [1.0, 37.0] table, and the script header still names mimiciv_hosp / mimiciv_icu as the defaults while the query body hardcodes mimiciv_3_1_icu / mimiciv_3_1_hosp, so both are worth syncing to the query that is actually committed.

[MohShahin]

Thanks for retracting the anchor_year_group approach instead of patching around it. The header comment in derive_ehr_resource_contexts.sql spelling out why the 102-to-37 drop was the pool shrinking rather than the number getting real is exactly the right way to record a wrong turn, and deriving within a single hadm_id does genuinely sidestep the shift problem.

One problem is that the new proxies no longer match what the adapter says they are: the query now emits beds as c.icu_stay_count and staffing as ROUND(1.0 / c.icu_stay_count, 4), while SPEC.raw_hint and the ResourceContext docstring in ehr.py still describe bed occupancy and a nurse-to-patient ratio, and neither was updated in this commit. That leaves two of the three numeric constraint columns as algebraically one variable instead of two independent signals, and an admission with a single ICU stay lands on beds=1.0 and staffing=1.0, so the scrutiny panel reads a near-constant column where the resource-constraint signal is supposed to be, and a reader seeing beds=3 will take it as three beds rather than one patient moved through the ICU three times.

You have multiple options to fix this. For example, I would keep the within-admission count but name it for what it measures and put that name in REQUIRED_COLUMNS, leaving beds and staffing out until there is a source for real occupancy, rather than shipping two column names for one number under labels that mean something else. Smaller and separate: the PR description still presents the anchor_year_group bucketing as fix #1 alongside the beds [1.0, 37.0] table, and the script header still names mimiciv_hosp / mimiciv_icu as the defaults while the query body hardcodes mimiciv_3_1_icu / mimiciv_3_1_hosp, so both are worth syncing to the query that is actually committed.

Fixed. You're right — beds/staffing were shipping under labels that
didn't match what the number measured, and worse, they weren't even two
independent signals (staffing = 1/beds). Dropped both entirely and
replaced with a single honestly-named icu_stay_count column: distinct
ICU stays per hadm_id, in REQUIRED_COLUMNS, the ResourceContext
dataclass, SPEC.raw_hint, and the module docstring. No more "beds" or
"staffing" anywhere in the loader.

Re-validated the full schema change against a fresh real export:
94,382 contexts, zero parse errors.

icu_stay_count distribution:

icu_stay_count	count	%
1	77,475	82.1%
2	13,068	13.8%
3	2,718	2.9%
4	704	0.7%
5	280	0.3%
6	90	0.1%
7	28	<0.1%
9	9	<0.1%
10	10	<0.1%

Same smooth single-to-multi-stay decay as the earlier beds numbers,
just under an honest name and grounded in a real within-admission count
this time, not a cross-patient collision.

Also fixed the two smaller items: the PR description no longer presents
anchor_year_group bucketing as current (rewritten below), and the
script header now names mimiciv_3_1_hosp/mimiciv_3_1_icu to match
what the query body actually queries and what this was validated against,
with the unversioned default noted as the substitution for v2.2 access.

Thanks for pushing on this twice — the shipped schema is meaningfully
more honest for it.

[MohShahin]

Good real-data validation, and the empty-string and LOS fixes are solid catches. One issue on beds/staffing though: MIMIC-IV shifts dates per patient, so same anchor_year_group doesn't mean actually concurrent, and right now that's what's driving the numbers. Agastya's idea of using ICU-stay-count or LOS per admission instead sounds like the right fix, let's hold off on beds/staffing until that's sorted.

Fixed per the thread with @Agastya191 above beds/staffing are gone,
replaced with a single honestly-named icu_stay_count column derived
entirely within hadm_id. Re-validated against a real 94,382-row export,
numbers in my reply to Agastya. Let me know if you want anything else
before this is ready for another pass.

[Agastya191]

Good work on the retraction in derive_ehr_resource_contexts.sql and the schema cleanup in ehr.py — deriving within hadm_id rather than patching the anchor_year_group bucketing is the right response, and collapsing beds/staffing into a single icu_stay_count removes the two-names-one-number problem cleanly.

One problem: in scripts/mimic_iv/derive_ehr_resource_contexts.sql the final SELECT emits one row per icu.stay_id, but both numeric fields the loader reads are keyed to hadm_id (icu_stay_count from the grouped CTE, budget_pressure from admission_info), so they are constant across every stay in an admission. That makes 9,214 of the 94,382 rows exact payload duplicates and size-biases the file by exactly the load variable: admissions with three or more ICU stays are 4.07% of rows but 1.36% of admissions, and the mean icu_stay_count reads 1.24 rather than 1.11. The distribution table in your description is itself the evidence — every bucket divides exactly by its own count (13,068/2, 2,718/3, 704/4, … 10/10), which only happens if each admission contributed k identical rows — and since hadm_id is not in the SELECT, a consumer cannot group it back out.

You have a few options here. I would emit one row per hadm_id, since that is the grain both numbers actually describe; if you want to keep the stay grain for the careunit variation, adding hadm_id as a column would at least let callers weight or dedupe.

Separately, docs/PIPELINE.md line 84 and the ehr layout string in benchmaxxing/datasets/staging.py still describe "bed occupancy, staffing, budget pressure" — they are the last two places carrying the schema this PR removed.

[sebasmos]

This is exactly the right call, dropping beds/staffing rather than patching around what they actually measured. The icu_stay_count derivation and the doc sync are both clean, and 94k real rows with zero errors is solid validation. Approving.

[MohShahin]

Good work on the retraction in derive_ehr_resource_contexts.sql and the schema cleanup in ehr.py — deriving within hadm_id rather than patching the anchor_year_group bucketing is the right response, and collapsing beds/staffing into a single icu_stay_count removes the two-names-one-number problem cleanly.

One problem: in scripts/mimic_iv/derive_ehr_resource_contexts.sql the final SELECT emits one row per icu.stay_id, but both numeric fields the loader reads are keyed to hadm_id (icu_stay_count from the grouped CTE, budget_pressure from admission_info), so they are constant across every stay in an admission. That makes 9,214 of the 94,382 rows exact payload duplicates and size-biases the file by exactly the load variable: admissions with three or more ICU stays are 4.07% of rows but 1.36% of admissions, and the mean icu_stay_count reads 1.24 rather than 1.11. The distribution table in your description is itself the evidence — every bucket divides exactly by its own count (13,068/2, 2,718/3, 704/4, … 10/10), which only happens if each admission contributed k identical rows — and since hadm_id is not in the SELECT, a consumer cannot group it back out.

You have a few options here. I would emit one row per hadm_id, since that is the grain both numbers actually describe; if you want to keep the stay grain for the careunit variation, adding hadm_id as a column would at least let callers weight or dedupe.

Separately, docs/PIPELINE.md line 84 and the ehr layout string in benchmaxxing/datasets/staging.py still describe "bed occupancy, staffing, budget pressure" — they are the last two places carrying the schema this PR removed.

Confirmed, good catch — and the arithmetic proof from the distribution
table was clean, thank you for spelling it out. Fixed exactly as
suggested: the query now emits one row per hadm_id (renamed
scenario_id accordingly), which is the actual grain both
icu_stay_count and budget_pressure describe. Also added
ROW_NUMBER() OVER (PARTITION BY hadm_id ORDER BY intime) to pick a
single representative careunit per admission (the chronologically
first ICU stay) rather than losing that column or arbitrarily picking one
-- noted as a known simplification in the script header, since multi-stay
admissions crossing careunits won't have that variation captured.

Re-validated against a fresh real export:

• 85,168 rows (true admission count, down from 94,382 stay-level rows)
• 0 duplicate scenario_id
• mean icu_stay_count = 1.108 -- matches your calculated true value of
  1.11 almost exactly
• distribution: 91.0% single-stay, 7.7% two-stay, 1.1% three-stay,
  tapering to a single admission at 9 and one at 10

Also synced the two doc references you flagged (docs/PIPELINE.md line
84, staging.py's ehr layout string) -- both now say "per-admission
ICU-stay count, budget pressure" instead of the removed "bed occupancy,
staffing" schema.

Thanks for the careful review across all three rounds -- the shipped
version is meaningfully more correct for it. Since this fixes a real data
bug in already-approved code rather than a style note, flagging in case
you want to re-review before merge rather than relying on the standing
approval.

[sebasmos]

Grain fix looks right, one row per hadm_id and the docs are all synced. Two things before this goes in:

1. The PR description still shows the old 94,382 rows and the inflated distribution. Please update it to 85,168 and mean 1.108, otherwise the merged description is the artifact Agastya flagged.
2. first_stay's ROW_NUMBER() OVER (PARTITION BY hadm_id ORDER BY intime) has no tie-break, so careunit is nondeterministic when an admission has two stays sharing an intime. Adding intime, stay_id fixes it.

Also flagging a process thing on me: my earlier approval was at 08:29 and your fix commits landed at 11:21 and 11:27, so that approval covered the pre-fix SQL and should not be treated as sign-off on what is on head now. @Agastya191 should get a look at the regrain since he found the original.

[sebasmos]

Pushed 557ff9c: the first_stay ROW_NUMBER now orders by intime, stay_id, so careunit is deterministic when two stays share an intime. Verified the grain regrain itself is genuinely done. Only thing left is updating the PR description off the stale 94,382-row figure to 85,168 rows and mean 1.108.