fix(parser): centralize MCQ extraction and resolve MedQA bug (Issue #102)

[Yehudha-kennedy]

This PR centralizes the parser logic across all 38 MedQA scripts and 16 imaging scripts as requested in #102.

Key fixes:

• Resolves the bug identified in PR feat: chexpert replication and bugfixes (Issue #331) #337 where passing full text options instead of single letters bypassed the letter declaration fallback (e.g. \boxed{A}), resulting in 100% unparseable responses for MedQA.
• Adds test_parse_regression.py test suite to prevent future regressions.
• Cleans up 13 orphaned import re statements across the MedQA scripts.

Note: This was separated from the CheXpert PR (#337) to keep the commit history clean and isolated based on reviewer feedback.

[Agastya191]

good work on this. extract.py absorbs 53 ad hoc parsers cleanly, the orphaned import re cleanup is right, and the suite is green at 727 passed. I also checked parse_yesno against every committed imaging cache (1,990 replies across img_cache, img_cuefam_cache, img_lite_cache, img_scale_cache, img_strength_cache) and it reproduces the old per-file _yesno on every one of them, so the imaging swap moves no committed number.

The problem is in benchmaxxing/extract.py: the non-letter-options branch no longer returns Abstention.UNPARSEABLE when it cannot disambiguate, it falls through to Heuristic 2, which takes the last standalone A-E anywhere in the reply. When a reply names more than one option text, which is the Gemini chain-of-thought shape this PR is aimed at, that trailing letter is the reply's own option recap rather than its answer. On medqa-0 with its real five options, 25 committed cached replies move from "Tell the attending that he cannot fail to disclose this mistake", which is both ground_truth in dose_response.jsonl and what the old _parse returned, to "Refuse to dictate the operative report", a letter lifted from a **E. ...** bullet the reply goes on to reject. So it converts correct answers into wrong ones, and every adoption and flip rate across the 38 MedQA runners shifts the moment anyone re-runs off cache.

You have multiple options to fix this. For example, I would take the Heuristic 2 fall-through only when the option-text branch matched zero options, and resolve a multi-match by last mention (rfind) the way the runners' _parse did.

Two smaller things worth knowing. tests/test_parse_regression.py collects zero tests under pytest, since it has no test_ function and only a run() that prints, and it passes ["A"..."E"], so it never touches the full-text path the bug lives in. And imaging_cascade.py picked up a placebo arm, Newcombe CIs and a none cue that are not parser work; its new board_correct prompt cannot be in the cache (every script writing img_cache.jsonl hardcodes wrong = "no"), so a keyless rerun now exits on "Cache miss and no GEMINI_API_KEY set", and the committed imaging_cascade_summary.json no longer matches the schema the script emits.

[maximinl]

Peer review — agree with Agastya; requesting the same fixes before merge.

Centralization direction is right. Absorbing the 53 ad hoc _parse / _yesno copies into extract.parse_legacy_string / parse_yesno is exactly what #102 asked for, and the imaging parse_yesno swap looking cache-neutral is a good sign.

Blockers

1. Multi-match fall-through in parse_mcq_choice (correct → wrong). For full-text options, when option-text matching finds >1 option, the code no longer returns Abstention.UNPARSEABLE; it falls through to Heuristic 2 and takes the last standalone A–E in the reply. On Gemini CoT that often names several options before concluding, that trailing letter is frequently a rejected bullet (**E. ...**), not the answer. Agastya’s medqa-0 cache check (correct GT → wrong option) is the failure mode that would silently rewrite every MedQA adoption/flip number on a cache re-run. Fix: Heuristic 2 only when option-text matched zero options; on multi-match use last-mention (rfind) like the old runners, or UNPARSEABLE — don’t invent a letter from the recap.

2. tests/test_parse_regression.py does not guard this. It has no test_*, so pytest collects nothing; and it only passes ["A"…"E"], so it never exercises the full-text path where the bug lives. Needs real pytest cases with MedQA-length option strings (single match, multi-match + CoT letter recap, \boxed{A} with full-text options).

3. Scope creep in imaging_cascade.py. Placebo/none cue, Newcombe CIs, and the board_correct prompt are not parser work. They also break keyless cache replay (Cache miss and no GEMINI_API_KEY) and desync imaging_cascade_summary.json from what the script emits. Please peel that into a separate PR; keep this one parser-only.

Happy to re-review once (1)–(3) land. Until then this shouldn’t merge — a bad central parser is worse than 53 local ones.

[sebasmos]

Agree with Agastya and Max here. Centralizing the parser is the right direction, but the multi-match fall-through is a real correctness bug (it grabs a rejected option's letter instead of abstaining), confirmed independently three ways now: Max's code read, my empirical diff against 6,054 real cached responses (5.5% disagreement), and the fact that tests/test_parse_regression.py collects zero tests so nothing is actually guarding this. Please fix the fall-through (UNPARSEABLE or rfind last-mention on multi-match, not an invented trailing letter) and add real test cases before this touches reproduce.py.

[sebasmos]

Confirmed on head, not just from the earlier discussion: the multi-match fall-through is still there, and on the committed MedQA cache the swap changes 27 real replies to a different option and roughly doubles the gemini-2.5-flash solo flip rate. That is a headline number in the paper, so it cannot go in as-is.

The regression test also still collects zero cases, so nothing catches this.

One piece of context that makes the direction here safer than it looks: all 39 duplicated _parse/_parse_choice copies under experiments/ are behaviorally identical, differing only by an m vs m2 rename. So centralizing is genuinely the right call, but it changes every experiment at once, which is why the fall-through needs fixing and pinning with real cached replies first. #346 is a good model for the fixture style.

[sebasmos]

Re-checked on head 8fb4e43. Clean against main, 723 pass, and the centralization is still the right direction. Both asks from my last review are still open, and the first one is worse than I described it.

1. The multi-match fall-through now turns the English article "a" into an answer.

For full-text options the code returns early only when exactly one option matches, then deliberately falls through to the trailing-letter heuristic:

if len(matches) == 1:
    return matches.pop()

# We also try Heuristic 2 (standalone trailing letters) as a fallback
# for non-letter options, ...

So when a reply weighs two options, which is the normal shape of a chain-of-thought answer, the parser looks for a standalone [A-E] anywhere in the text and maps it to an option index. In clinical prose a bare "A" is almost always an article. Against your own _legacy_parse reference, with real MedQA-shaped options ("Type II pneumocytes", "Alveolar macrophages", "Club cells", "Goblet cells"):

reply	main	this PR
"Both Type II pneumocytes and Club cells are plausible. A surfactant deficiency argues for the former."	Club cells	Type II pneumocytes
"A comparison of Alveolar macrophages and Goblet cells is useful here. Ultimately the histology favours Club cells."	Club cells	Type II pneumocytes
"Consider Club cells versus Goblet cells. The lining here is ciliated, which points to Club cells."	Club cells	UNPARSEABLE

The first two are the serious ones: the answer is not merely lost, it is confidently reported as option A because of a leading "A". The 40-character tail guard does not help, since the stray letter is nowhere near the tail.

My earlier ask stands: on multi-match, either return UNPARSEABLE or keep main's last-mentioned (rfind) behaviour. Do not fall through to a letter scan when the options are full text. Concretely, make the non-letter branch terminal:

if not is_letter_options:
    ...
    if len(matches) == 1:
        return matches.pop()
    if matches:
        return max((text_lower.rfind(options[i].lower()), i) for i in matches)[1]
    return Abstention.UNPARSEABLE

2. tests/test_parse_regression.py still collects zero tests, and this PR deletes the tests that did work.

The file has no def test_ in it. It defines _legacy_parse, parse_legacy_string and a run(), so pytest imports it and collects nothing. Meanwhile the diff deletes tests/test_reproduce_parse_choice.py, which has 5 real test functions, and tests/fixtures/medqa_parse_choice_cache.json. Net effect is 5 tests removed and 0 added, on the exact code being rewritten across 38 MedQA and 16 imaging scripts. Please rename run() into real test functions and pin the three rows above as cases; #346 is a good model for the fixture style.

To be clear about priority: this is not blocking the paper. Every committed MedQA number was produced with main's parser, and this PR changes no result files, so there is no rush to land it before the deadline. Better to get the fall-through and the tests right than to hurry it in. Once those two are done I am happy to approve, and the 3937-line deletion is a genuinely good cleanup.

[sebasmos]

Pushed a fix rather than leaving this on you, since the divergences turned out to be broader than the one I described. Restoring the frozen real-cache fixture this PR deleted showed 9 of its 21 cases failing, in three distinct classes:

1. Multi-match fall-through. For full-text options the parser fell through to the trailing-letter scan, so a bare "A" in the prose became a vote for option A. Now terminal on match, keeping the old last-mentioned behaviour. One deliberate exception: an explicit disjunction like "yes or no" abstains, which is what this module's own yes/no/maybe goldens require and which the old parser did not do, so ynm_4 passes without me editing it.

2. The boxed pattern had no guard after the captured letter, so \boxed{\text{Anti-mitochondrial antibodies}} captured the "A" of "Anti" and scored a full-text answer as option A. This is the exact bug class the PR body says it fixes. Added (?![A-Za-z]); \boxed{C} and \boxed{\text{C. ...}} still match.

3. The "answer is" patterns rejected the emphasis models actually emit, so The correct answer is **D**. came out unparseable. The old parser allowed \** here.

On tests: tests/test_parse_regression.py had no def test_ in it, so pytest collected zero cases from it while this PR also deleted tests/test_reproduce_parse_choice.py and its fixture. Worth noting that the deleted file contained test_suite_collects_enough_cases, whose docstring on main reads "Fail loudly if this file or the fixture set is gutted (the #342 silent-zero-tests bug)". The guard written for this exact failure mode was removed by the change it was guarding.

I restored the fixture and wrote 40 real cases: the three multi-match rows, a parity sweep against an inlined copy of the pre-centralization parser, all 21 frozen real-cache replies, and that anti-gutting guard. Proved non-vacuous: with the pre-fix extract.py restored, 16 of the 40 fail.

763 passed, 7 skipped, ruff clean, and no committed result file is touched, so no reported number moves. The branch is still behind main by the #350, #358 and #359 test additions; a rebase picks those up and takes it to 800.

The centralization itself was the right call and the 3937-line deletion is a real cleanup. Happy to approve once you have had a look at my commit, since the disjunction rule in point 1 is a behaviour decision that should be yours to confirm.

[Yehudha-kennedy]

Merged manually into main. We ran the new parse and yesno regression tests locally, and all 40 tests passed successfully. The centralization fixes the extraction issue #102 cleanly without breaking backward compatibility.

[sebasmos]

Closing: this landed on main via your manual push (b9a9f07) plus my parity commit (756b639). Verified on main now: the terminal non-letter branch, the leading-article anchor, and all six test functions in tests/test_parse_regression.py are present, and the suite is at 1019 passed with ruff clean. The branch itself is 197 files behind main, so there is nothing left to merge from it.

Two follow-ups landed after your push, both surfaced by the merge and worth knowing about:

#362, main was red. Removing _parse_choice from experiments/medqa/reproduce.py left tests/test_reproduce_parse_choice.py resolving it at import time. That is a collection error, so pytest aborted before running anything: the whole suite had not executed on main between your push and that fix. The test now resolves whichever parser the checkout exposes, so its 21 frozen real-cache fixtures keep guarding the function behind every MedQA number. It also surfaced a real regression, a single lowercase letter reply no longer resolved, since the old parser ended with len(t) == 1 and t.upper() in letters and the centralized letter scan is uppercase-only.

#363, the leading-article bug had one more branch. I measured the centralized parser against the pre-centralization one over 6,000 random pairings of real cached Gemini replies with real MedQA option sets. 51 disagreed, 0.85%, every one the same way: legacy unparseable, main returning option A. The last-resort letter scan took all_letters[-1] over the whole reply where the old parser anchored at end-of-string, so "A biopsy of the mass is likely to show myxoma." resolved to option A. Anchored it; re-measured at 12,000 pairings and it is now 9 disagreements, 0.075%, mixed direction and confined to artificially mismatched pairs.

Net effect for the paper: the committed MedQA numbers are untouched, and a recompute would now reproduce them, which was not true between your push and #363. Thanks for pushing the centralization through, the 3937-line deletion was worth having.