Skip to content

Add diagnostic-delay concept module (cohort + delay measures + data-quality report) - #2173

Open
developer-rpai wants to merge 2 commits into
MIT-LCP:mainfrom
developer-rpai:feature/diagnostic-delay-cohort
Open

developer-rpai wants to merge 2 commits into
MIT-LCP:mainfrom
developer-rpai:feature/diagnostic-delay-cohort

Conversation

@developer-rpai

Copy link
Copy Markdown

What this is

A new concept module, mimic-iv/concepts/diagnosticdelay/, for reproducible cohort extraction and diagnostic-delay measurement on MIMIC-IV:

  • suspected_sepsis_cohort.sql — one row per ICU stay: adults admitted via the emergency department with at least one suspected-infection episode (antibiotic + microbiology culture pair, per suspicion_of_infection), with anchor timestamps and demographics.
  • diagnostic_delay.sql — delay in hours between hospital/ICU admission and first infection recognition (suspected-infection time, first antibiotic, first culture); an onset-window classification (pre_admission / present_on_admission / hospital_onset); and temporal-plausibility flags (1 = implausible, NULL = unassessable) for out-of-order or missing timestamps.
  • data_quality_report.py — generates a Markdown/JSON data-quality report (cohort coverage, missingness, flag prevalence, delay distributions with median/p25/p75) from the two tables; runs on DuckDB or PostgreSQL with portable SQL.
  • README.md + concept-index entry — documents the cohort definition, measures, validation, and limitations.

Why

ICD codes in MIMIC carry no timestamp, so "when was this diagnosed relative to admission?" is a recurring community question (e.g. #1843 on diagnosis timing for sepsis/AKI research). This module answers a tractable version of it by proxying diagnosis time with the clinically observable recognition events already used in the Sepsis-3 definition. The plausibility flags address the class of problem raised in #2168 (implausible values invisible to anyone who doesn't check before aggregating): flagged stays can be investigated rather than silently averaged in.

Validation (what ran, what didn't)

I do not have credentialed MIMIC-IV access, so validation was done without real data — an end-to-end run on MIMIC-IV (v3.1) is still outstanding:

  1. sqlglot parse check of both queries in the BigQuery dialect (.github/scripts/check_sql_syntax.py) — pass.
  2. sqlfluff lint with the repo's .sqlfluff config (sqlfluff 4.1.0, same as CI) — pass, no violations.
  3. Transpiled both queries BigQuery → DuckDB and BigQuery → PostgreSQL with the repo's mimic_utils transpiler; both parse in the target dialects.
  4. Executed the DuckDB build against hand-built synthetic fixtures mirroring the MIMIC-IV schema (patients, admissions, icustays, derived suspicion_of_infection): 16 assertions covering a standard present-on-admission case, a hospital-onset case, a pre-admission-suspicion case (flagged), multiple episodes per stay, pediatric exclusion, non-ED admission exclusion, a stay with no suspected infection (excluded), and a NULL icu_outtime case (flag unassessable) — all pass, delays match hand computation.
  5. Ran data_quality_report.py against the fixture database (with a raw-schema coverage denominator): Markdown/JSON outputs verified against hand-computed values (4/5 = 80% capture, flag counts, medians/p25/p75).

Notes for reviewers

  • Only mimic-iv/concepts/ sources are touched. The transpiled concepts_postgres/concepts_duckdb outputs are intentionally not included — the regenerate-dialects bot produces those on merge. The duckdb.sql/postgres-make-concepts.sql build lists are likewise left for maintainers to extend.
  • The module depends only on mimiciv_hosp + mimiciv_icu + derived concepts (no MIMIC-IV-ED module), so it runs on the standard build. ED arrival time is deliberately not used; the README notes this as a limitation.
  • Draft PR — happy to adjust naming, column choices, or the onset-window thresholds to fit maintainer conventions.

…uality report)

New `mimic-iv/concepts/diagnosticdelay/` module:

- suspected_sepsis_cohort.sql: one row per ICU stay for adults admitted
  via the ED with >= 1 suspected-infection episode (antibiotic +
  microbiology culture pair, per suspicion_of_infection).
- diagnostic_delay.sql: delay in hours between admission (hospital/ICU)
  and first recognition of infection (suspected-infection time, first
  antibiotic, first culture); onset-window classification
  (pre_admission / present_on_admission / hospital_onset); and
  temporal-plausibility flags for out-of-order or missing timestamps.
- data_quality_report.py: renders a Markdown/JSON data-quality report
  (cohort coverage, missingness, flag prevalence, delay distributions)
  from the two tables above; runs on DuckDB or PostgreSQL.
- README.md documents the cohort definition, measures, validation,
  and limitations.

ICD codes carry no timestamp in MIMIC, so diagnosis time is proxied by
the clinically observable recognition events used in the Sepsis-3
definition (see MIT-LCP#1843); ordering checks surface implausible values of
the kind reported in MIT-LCP#2168 instead of silently averaging them in.

Validated without credentialed MIMIC-IV access: sqlglot parse (BigQuery
dialect, same as CI), sqlfluff 4.1.0 with the repo config, transpile to
DuckDB/PostgreSQL via mimic_utils, execution of the DuckDB build on
hand-built synthetic fixtures (16 assertions), and a run of the report
script against the fixture database. End-to-end run on real MIMIC-IV
data is still outstanding.

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

Please fix the two report-generator issues below before this is merged. I checked the existing discussion (no prior review or inline comments) and reproduced the schema failure with synthetic DuckDB tables matching the repository layout. The PostgreSQL API mismatch is also confirmed against psycopg2 2.9.13. Please add regression coverage for both backends and the split ICU/hospital schemas.



def fetchone(con, sql, params=()):
cur = con.execute(sql, params)

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.

[P1] Use a DB-API cursor for PostgreSQL queries. connect() returns a psycopg2 connection for --postgres-dsn, but that connection has no execute method. The first table_exists() call catches the resulting AttributeError and exits with a misleading "table ... not found" message even when both concept tables exist, so the advertised PostgreSQL mode cannot generate any report. Please execute/fetch through a cursor in both query helpers (and close it), and test the PostgreSQL path. Psycopg2 documents this interface at https://www.psycopg.org/docs/connection.html#connection.cursor.

SELECT COUNT(*) AS n_adult_ed_icu_stays
, COUNT(DISTINCT ie.subject_id) AS n_patients
FROM {raw_schema}.icustays ie
INNER JOIN {raw_schema}.admissions adm

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.

[P2] Query admissions and patients from the hospital schema. The documented --raw-schema mimiciv_icu locates icustays, but the repository builds admissions and patients in mimiciv_hosp. Consequently this join fails on a standard build, the broad exception handler returns None, and the requested coverage denominator silently disappears. I reproduced this with the three tables in their normal schemas. Please accept separate ICU/hospital schema names (or otherwise map both) and test the documented invocation against that layout.

…it ICU/hospital schemas

- [P1] fetchone/fetchall now route through an explicit DB-API cursor when
  the connection has no execute() method (psycopg2), closing the cursor
  afterwards; DuckDB connections still execute directly.
- [P2] coverage_stats accepts separate ICU and hospital schema names via
  new --raw-hosp-schema flag (defaults to --raw-schema); the coverage
  denominator now joins mimiciv_icu.icustays to
  mimiciv_hosp.admissions/patients instead of failing silently.
- Add test_data_quality_report.py with regression coverage for both
  backends (fake psycopg2 connection + in-memory DuckDB) and the split
  ICU/hospital schema layout.
@developer-rpai

Copy link
Copy Markdown
Author

Both issues fixed in the latest push (97d9e77):

[P1] PostgreSQL cursor: fetchone/fetchall now go through _cursor(), which returns an explicit DB-API cursor when the connection has no execute method (psycopg2) and closes it afterwards; DuckDB connections still execute directly. The misleading "table not found" on the PostgreSQL path is gone because the query actually runs now.

[P2] Split schemas: new --raw-hosp-schema flag (defaults to --raw-schema for backwards compatibility). The coverage denominator now joins {icu}.icustays to {hosp}.admissions/{hosp}.patients, so the documented invocation is --raw-schema mimiciv_icu --raw-hosp-schema mimiciv_hosp.

Regression coverage: added test_data_quality_report.py (6 tests, all passing):

  • Fake psycopg2-style connection (has cursor(), no execute()) verifies queries route through the cursor and it is closed
  • In-memory DuckDB with the standard split layout (mimiciv_icu.icustays + mimiciv_hosp.admissions/patients) verifies the coverage denominator and a full end-to-end report
  • Single-schema backwards compatibility (--raw-hosp-schema omitted)

@developer-rpai
developer-rpai marked this pull request as ready for review October 2, 2026 22:54

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

Rechecked the updated commit. All six added tests pass, and the previous cursor and split-schema findings are addressed. I also ran the actual transpiled concept SQL against synthetic data on PostgreSQL 17 with psycopg2. That exposes a separate blocker in numeric quantile handling, detailed below; please add a regression exercising PostgreSQL-returned numeric values.

pos = q * (len(vals) - 1)
lo = math.floor(pos)
hi = math.ceil(pos)
out[q] = vals[lo] + (vals[hi] - vals[lo]) * (pos - lo)

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.

[P1] Handle PostgreSQL Decimal values when interpolating quantiles. The transpiled diagnostic_delay SQL uses ROUND(CAST(... AS NUMERIC), 1), so psycopg2 returns decimal.Decimal for all five delay measures. This line multiplies their Decimal difference by a float weight and raises TypeError, even for a single populated stay. I reproduced the complete path by transpiling both concept queries with mimic_utils, building them on PostgreSQL 17, and calling build_report: admission_to_suspicion_hours was Decimal("2.0"), then delay_distributions -> quantiles failed with "unsupported operand type(s) for *: decimal.Decimal and float". Thus --postgres-dsn still cannot produce the report despite the corrected cursor path. Please normalize numeric values or use compatible interpolation weights, and add a regression using actual PostgreSQL NUMERIC results (the current fake cursor returns only integers and misses this).

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