Skip to content

feat: committed runner for the MIMIC-CXR imaging battery (#343) - #347

Merged
sebasmos merged 0 commit into
mainfrom
fix/mimic-battery-runner
Jul 29, 2026
Merged

feat: committed runner for the MIMIC-CXR imaging battery (#343)#347
sebasmos merged 0 commit into
mainfrom
fix/mimic-battery-runner

Conversation

@Agastya191

Copy link
Copy Markdown
Collaborator

Closes #343. Stacked on #329, so the base is feat/mimic-subset (the only branch where experiments/mimic_cxr_image/ exists). It needs a rebase onto main before merge to pick up #338, and the runner enforces that (see below).

What was actually missing

The issue reads as "no committed script calls the Gemini API for MIMIC". That is half right. The arms were never run by an absent MIMIC-specific script: they were run by the shared, dataset-agnostic runners in experiments/imaging/, which already take --manifest --image-root --out --cache --n. What was missing is the binding: which manifest, which out dir, which cache, in what order. That lived only in README prose, so the committed summaries could not be regenerated or audited from the repo.

So this does not fork five copies of the imaging runners into experiments/mimic_cxr_image/. Forking would recreate the exact bug class #332/#333/#338 had to fix across seven sibling files, and it runs against the direction the CheXpert side already took (#331's v2 refactor deleted its forked runners in favour of the shared ones). run_battery.py is the committed binding to the already-fixed shared runners instead.

python -m experiments.mimic_cxr_image.run_battery --image-root ~/mimic-cxr-raw
python -m experiments.mimic_cxr_image.run_battery --image-root ~/mimic-cxr-raw --arm cascade
python -m experiments.mimic_cxr_image.run_battery --image-root . --dry-run   # no key, no images

Three defects in the documented recipe

Verifying the README's commands turned up bugs, not just missing code:

  • Cache leak. Every runner except imaging_solo defaults --cache into experiments/imaging/results/, which holds the NIH lane's committed caches. The README commands never pass --cache, so a MIMIC run appends credentialed-report-derived reads into tracked files. All arms now share one cache under the MIMIC results dir, which also lets the nested arms (blind 100 < cascade 150 < referee 300 < solo 600) reuse each other's identical clean reads.
  • Silent truncation. --n defaults to the NIH arm sizes (35 cascade, 40 solo). The documented --n 9999 clears today's 834-image arm only by luck; the runner passes a bound no arm reaches.
  • A silently vanishing result. imaging_system_flag computes vs_peer_assertion_cascade by reading imaging_cascade.jsonl from its own --out. Once the cascade moved to cascade_150/ in b8b4905, that block drops out of the summary with no warning. The runner stages the transcript there first.

The runner also refuses to launch any cascade-family arm whose runner still plants wrong = flip(clean_read) rather than against ground truth. On this branch that correctly blocks system_flag and strength_cascade, which main fixed in #338 and this branch picks up on rebase.

The committed summaries are stale, and cannot be otherwise

#343's fourth checklist item has a definite answer. results/plant_direction_summary.json records ~80% of MIMIC cases planting the truth, which is impossible under the corrected wrong = "no" on an all-finding-present cohort. Five of the nine committed summaries are affected: the four cascade-family arms plus referee_300/imaging_referee_summary.json, which scores a replay of the cascade transcript. imaging_solo_summary.json, nih_match_35/imaging_solo_summary.json and imaging_blind_metric_summary.json plant nothing and reproduce as committed.

arm committed (ALL, pre-fix) genuinely-false subgroup % that planted the truth
cascade (150) +0.819 +0.488 (n=43) 80.0%
referee_cascade (300) +0.808 +0.506 (n=87) 79.1%
system_flag +0.200 +0.225 (n=169) 79.7%
strength_cascade 0.15 / 0.30 / 0.45 +0.797 / +0.808 / +0.795 +0.548 / +0.531 / +0.553 78.5%

The README now carries this table with the caveat that the genuinely-false column is a floor, not a prediction of the rerun: it is measured only on the ~20% of cases whose clean read was already correct, and on the other ~80% the holdout's own contaminated read tends to agree with the planted "no", which raises isolated adoption and pushes contagion (shared minus isolated) down. Only the rerun settles where the full arm lands.

Verified

  • 13 new tests in tests/test_mimic_battery.py plus the 6 existing test_mimic_subset.py tests pass, ruff clean on every file touched.
  • The coverage test reads the real committed *_summary.json files rather than a hand-copied list, so a future summary with no arm behind it fails there instead of becoming the next unreproducible number.
  • All 8 generated command lines were executed against fixture manifests to confirm the shared runners actually accept them: each gets past argparse and manifest loading and fails only on the absent images.

What still needs a real run, and the ask

@MohShahin the one thing I cannot do here is #343's real-data rule: I have no MIMIC-CXR images on this machine, so the five affected arms have not been regenerated through the real Gemini API. You already work against credentialed PhysioNet data on #340, so if your access covers MIMIC-CXR-JPG v2.1.0 and MIMIC-CXR v2.0.0 (a separate DUA from MIMIC-IV, so please check before committing to it), would you be willing to run this?

# 1. select the subset (metadata CSVs only, no images needed)
python -m experiments.mimic_cxr_image.build_subset select \
  --raw-root ~/mimic-cxr-raw --out-dir experiments/mimic_cxr_image/manifests --seed 20240724

# 2. download only the ~834 selected images (README section 2)
# 3. run every arm
export GEMINI_API_KEY=...
python -m experiments.mimic_cxr_image.run_battery --image-root ~/mimic-cxr-raw

Two things worth knowing before you start. Rebase onto main first or the runner will refuse system_flag and strength_cascade by design, since this branch predates #338. And --dry-run prints every command without spending a call, so it is worth one pass to confirm the paths resolve on your machine first.

If the credentials do not cover MIMIC-CXR, say so and I will find another owner rather than leave it assumed.

Once those numbers land, the committed cascade-family summaries should be replaced with them and the paper's MIMIC cells updated; plant_direction_check.py then becomes a no-op by construction and is kept only to re-derive the split from an archived pre-fix transcript.

Related: #295, #329, #331, #332, #333, #337, #338, #339.

@amarzullo24 amarzullo24 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving -- this closes the committed-runner gap on our branch the right way.

Binding to the shared experiments/imaging/ runners instead of forking is the correct call (the fork is exactly what gave #332 seven files to fix), and the clean_read_planters launch guard enforces the #333 ground-truth-plant fix going forward rather than trusting the next author to remember it.

Two of the three defects trace to our side, and I'm glad they're caught here:

  • unpinned --cache leaking MIMIC reads into the NIH lane's committed caches -- we hit exactly this (the img_cache.jsonl churn);
  • vs_peer_assertion_cascade silently dropping since b8b4905 (our cascade-collision fix moved the cascade to its own --out) -- the transcript-staging fix is correct.

run_battery.py supersedes our local-only run_remote.sh, which was the root of #343's gap; I'll retire it.

On item 4 (the real-data rerun): we have the PhysioNet imaging release, the key, and a working setup locally, so we'll take it ourselves rather than block on access. Running run_battery.py now and will post the regenerated summaries plus a committed-vs-regenerated diff so the fix is confirmed applied.

@amarzullo24

Copy link
Copy Markdown
Collaborator

@Agastya191 approved, thank you -- the binding-not-fork design + the launch guard are exactly right.

Heads-up before I touch the shared branch, since your PR is stacked on it:

To give the remote one coherent branch for the item-4 real regeneration (it needs latest main's fixed runners and the MIMIC tooling and run_battery.py together), I've locally rebased feat/mimic-subset onto latest main and cherry-picked your run_battery.py commit (f203fff, authorship preserved) on top. Verified: ruff clean, 35 tests pass, and run_battery --dry-run clears the plant-guard with --cache pinned and --n 1000000 across all eight arms. (Necessary because the current feat/mimic-subset base predates the #339 sibling fixes, so the guard correctly refuses system_flag/strength_cascade until it's rebased.)

The catch: publishing that means a force-push of feat/mimic-subset -- history rewritten. That changes #347's base and empties its diff, so it would be closed-as-folded-into-#329 rather than merged normally. I didn't want to force-push a branch your PR sits on without checking.

Your call, since it's your PR:

  1. I force-push the combined branch and you close feat: committed runner for the MIMIC-CXR imaging battery (#343) #347 as folded into MIMIC-CXR: deterministic subset tooling + imaging battery (closes #324) #329; or
  2. you rebase feat: committed runner for the MIMIC-CXR imaging battery (#343) #347 onto the updated main yourself and we merge it the normal way.

Either is fine by me. Separately: we'll take the item-4 real Gemini rerun on our side (we have the PhysioNet imaging release + key + a working setup), and post the regenerated summaries plus a committed-vs-regenerated diff so the fix is confirmed applied.

@sebasmos sebasmos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified the three README defects are all real, and the binding-not-forking design is the right call.

Retracted 2026-07-29. The plant-direction table below was true when I wrote it and is no longer true. imaging_system_flag.py and imaging_strength_cascade.py on the current head e6b8813 both read wrong = "no" with the #332/#339 comment, byte-identical to main. I re-ran this PR's own clean_read_planters guard against its tree: it refuses zero of 16 runners, not two. The "4 arms and 2 hard refusals" claim is obsolete, and the advice to merge #329 first then rebase is moot because this head already carries d3331d0.

What was actually blocking was the base branch, not the code. See my new comment.

The original review is kept below for the record.

Original review, 2026-07-28, now stale

This branch still carries the pre-#338 plant direction in two of the shared runners, so run_battery.py from here gives 4 arms and 2 hard refusals rather than a complete battery. Suggested order was: merge #329, rebase this onto main, then merge and run. Remaining ask was to drop Closes #343 until the real rerun lands.

@armaanvgrewal

armaanvgrewal commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

@sebasmos I resolved the conflict, but this PR has a few files causing it to fail the ruff check. Most likely because downstream main fixes aren't in this branch. Sharing since this may take longer to merge than I previously thought. I'll still be continuing to get this fixed first.

@sebasmos
sebasmos changed the base branch from feat/mimic-subset to main July 29, 2026 20:20
@sebasmos

Copy link
Copy Markdown
Member

Please hold before more work goes in: this needed no code changes.

I checked the head e6b8813 rather than the PR view, and main is already an ancestor of it. So this is a fast-forward of main: 10 commits, 16 files, git merge-tree clean, ruff check green. The plant direction is correct in all three runners, and this PR's own clean_read_planters guard refuses zero of 16 runners. It already carries d3331d0 (#329's nih_match fix) and f203fff (run_battery.py).

The only problem was the base. This PR targeted feat/mimic-subset, which is 24 commits behind main, and that is what inflated the diff to 74 files and broke the checks. I have retargeted the base to main. feat/mimic-subset is fully contained in this head so nothing is lost, and all authorship is preserved (6 commits @amarzullo24, 1 @Agastya191, 2 @armaanvgrewal). Merging with a merge commit rather than a squash so that stays true.

@amarzullo24 no force-push needed, and this does not have to be folded into #329. More importantly: you can start the item-4 real Gemini rerun right now from e6b8813. It already has latest main's fixed runners, the MIMIC tooling and run_battery.py together, which was your stated reason for wanting the combined branch. With the deadline in two days and the last long run taking over 29 hours, please do not wait on any merge for that.

@armaanvgrewal ruff check passes on the pushed head and there are no conflicts with main, so the failures you are hitting are local to your working tree. Nothing needs resolving here.

Note on Closes #343: merging this closes the issue on tooling alone, so I am reopening #343 afterwards. The real regeneration is still outstanding.

@sebasmos sebasmos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Fast-forward of main, guard clears all 16 runners, ruff green, authorship preserved. Merging with a merge commit, not a squash. Reopening #343 after, since the real regeneration has not run yet.

@sebasmos
sebasmos merged this pull request into main Jul 29, 2026
@sebasmos
sebasmos deleted the fix/mimic-battery-runner branch July 29, 2026 20:21
@armaanvgrewal

armaanvgrewal commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

@armaanvgrewal ruff check passes on the pushed head and there are no conflicts with main, so the failures you are hitting are local to your working tree. Nothing needs resolving here.

@sebasmos Initial ruff check . failed due to lint violations in assets/figures/figure1_gen.py. Determined the file was unrelated to the MIMIC runner work and fixed by merging main version with branch that already contained the fix (#341). Full details below.

Merge Conflict Resolution Summary

Objective

Resolve the #347 / #343 MIMIC-CXR imaging runner branch conflicts and bring the branch up to date with the latest repository state.

Actions Taken

Ruff Failure Investigation

  • Initial ruff check . failed due to lint violations in:
    • assets/figures/figure1_gen.py
  • Determined the file was unrelated to the MIMIC runner work.
  • Verified that current main already contained the fix (#341).
  • Merging origin/main brought in the existing fix and resolved the lint failures.

Validation

Passed all required checks:

ruff check .
pytest -q
benchmaxxing smoke

Results:

  • Ruff: ✅ Passed
  • Pytest: ✅ 782 passed, 6 skipped
  • Smoke: ✅ Completed successfully

Outcome

  • Merge conflicts resolved.
  • Branch updated with latest feat/mimic-subset and main.
  • All repository checks passed.
  • Changes pushed to:
fix/mimic-battery-runner
  • GitHub now reports:
No conflicts with base branch
Merging can be performed automatically

@Agastya191

Copy link
Copy Markdown
Collaborator Author

good work on run_battery.py and the tests. Binding to the shared experiments/imaging/ runners instead of forking them is the right call, the three README defects are all real (I checked the --cache defaults and confirmed only imaging_solo derives its cache from --out), and having the coverage test walk the real results/ tree rather than a hand-copied list is a better guard than the thing it replaces.

One problem is that test_every_committed_summary_has_an_arm_that_regenerates_it in tests/test_mimic_battery.py exempts plant_direction_summary.json, and the new .gitignore drops results/**/*.jsonl, so plant_direction_check.py has no committed input to read. That makes the +0.488 / +0.506 / +0.225 figures the README calls "the closest honest figure the committed data supports" the one set of numbers in the directory that cannot be re-derived from the repo, and by plant_direction_check.py's own docstring the pre-fix transcripts behind them will never exist again, so this is not a gap a rerun closes. The NIH lane does not have the problem because it commits its 28 transcripts under experiments/imaging/results/, which the MIMIC DUA will not let you copy here.

You have multiple options to fix this. What I would do is commit a reduced transcript carrying only wrong, iso_adopt and shared_adopt (plus the op*_ pair for the strength sweep) with case_id and image_ref stripped, which is non-credentialed and is exactly what analyze() reads, and then delete the exemption so the coverage test covers all nine summaries instead of eight.

sebasmos added a commit that referenced this pull request Aug 4, 2026
…attery

Fast-forward of main after retargeting the base off the stale feat/mimic-subset. Guard clears all 16 runners. Real regeneration still outstanding, #343 stays open.
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.

MIMIC-CXR imaging: no committed runner script for cascade/solo/system_flag/blind_metric (mirrors #337's CheXpert gap)

4 participants