fix: MIMIC-CXR text referee tautology + board-rationale bug (#373, #374) - #390
fix: MIMIC-CXR text referee tautology + board-rationale bug (#373, #374)#390felipeocampoos wants to merge 0 commit into
Conversation
|
@sebasmos — right on both counts, fixed. Falsifiability demonstration: ran the randomised-holdout check (d5ac221), same class as SUPPORT2's. Test count: confirmed — my earlier 957/18 was from a venv missing several optional extras (pillow, matplotlib, ruptures, statsmodels, pandas). With them installed I get 1051/5, matching your 1049/7 closely (small diff likely Thanks for keeping the oracle framing note — glad that part didn't need touching. And no worries at all on #357; the two-line check is exactly the kind of thing that's easy to skip under review time pressure, and catching it after the fact beats not catching it. |
Agastya191
left a comment
There was a problem hiding this comment.
good work on the #374 half, and the algebra holds up when checked against the committed rows: the old deployable is identical to adopted on all 40, and requery_ans has no structural tie to how wrong is picked, so the 1.0/1.0/0.0 is now a measurement rather than a guarantee. One problem is that the #373 half in referee_deployable.py and referee_judge.py rebuilds the board line inline from t.agent_id and t.answer behind a new --render-rationale flag, but #377 merged a few hours ago and already routes both of those exact files through blackboard.render_board with --show-rationale. Porting your two files onto current main fails test_no_module_rebuilds_the_answer_only_board_line_inline and test_every_audited_runner_goes_through_the_shared_renderer_with_the_knob, both of which name them by path, and it is the same collision that has this PR showing as conflicting. You have multiple options here, but I would drop the #373 half and rebase, keeping the referee predicate fix and the RESULTS.md relabelling, since that is the part main is still missing.
sebasmos
left a comment
There was a problem hiding this comment.
The predicate change is real. It does not make the number measurable, and the two figures the body leads with still cannot fail.
I verified this over the committed rows rather than reading the diff, so here are the counts first, n=40:
| check | count |
|---|---|
deployable != adopted |
0 of 40 |
the old tautological deployable (board != bare) != adopted |
0 of 40 |
oracle != adopted |
0 of 40 |
inferred_shortcut != wrong |
0 of 40 |
bare == wrong |
0 of 40 |
requery != bare |
4 of 40 |
requery == wrong |
0 of 40 |
| false positives | 0 |
| false negatives | 0 |
deployable is a subset of adopted |
true on 40 of 40 |
Why precision and FPR are still guaranteed. referee_deployable.py:147 picks wrong with the filter o != bare_ans, so bare_ans != wrong is true by construction and adopted at line 189 reduces to board_ans == wrong. The peers are hardcoded to wrong, so inferred == wrong always. Line 186 is therefore:
deployable = adopted AND (board_ans != requery_ans)
which makes deployable a strict subset of adopted. A false positive would need board == wrong and board != wrong at the same time. So FPR is identically 0.0 and precision identically 1.0, and only recall can move. That is the same structurally-zero comparator as before, moved one term to the right.
The falsifiability check only exercises recall. referee_deployable_falsifiability_check.py:60-61 reuses the real wrong and bare, so adopted still reduces to board == wrong and any disagreement can only be a false negative. All five disagreement_examples in the committed JSON are deployable: false, adopted: true. I re-ran your loop over 80,000 synthetic pairs and got zero false positives, every disagreement a false negative. So "falsifiable": true is true of recall and not of the precision and FPR pair it is cited to support. RESULTS.md's "the deployable referee's perfect score is now demonstrated to be genuine" overstates it, though credit where due, RESULTS.md does state plainly that deployable agrees with adopted on all 40 rows.
What would actually fix it, and it is one arm. #368 has the answer: an honest-peer clean-control arm where the peers assert the correct answer, so adopted is false by construction and a deployable flag there is a genuine false positive. On MedMCQA that turned a degenerate 1.000/1.000/0.000 into a measurable 0.742/1.000/0.140 with 8 real false positives. Rewording the predicate does not help; adding a satisfiable comparator does. Our full draft now cites that design as the general remedy for this family.
Note this would not fix recall, which stays 1.000 while every positive in the matrix is a planted row. Worth saying so in the table.
Three more, in descending order
1. Two more inline board renderers, which reddens the suite. referee_deployable.py:164-167 and referee_judge.py:164-167 rebuild the board inline. Main routes both through render_board since #377. Merged onto main with the conflict resolved your way, tests/test_board_render.py gives 2 failed, 9 passed, naming both files; full suite 2 failed, 1075 passed. The #373 half of this PR is already-done work on main, so the resolution is to drop your side of both hunks and keep render_board, which leaves the #374 half as the real content.
2. The judge lane was not re-run. referee_judge.py gains --render-rationale and a render_rationale summary field, but referee_judge_summary.json is not in the changed files and does not contain that key. So the "same-lineage judge 13/14" row is the unchanged #357 number, not a result of this PR. For that half the real-data rule is unmet.
3. Em dashes. 18 added, 4 removed, net +14, in referee_deployable.py:165, referee_judge.py:166, the oracle_upper_bound summary key, which carries one as an escaped \u2014, and RESULTS.md going 23 to 34. Against the standing rule.
Also: oracle at line 192 is still character-identical to adopted at line 189. Relabelling it definitional and pulling it out of the compared set is the right call and I would keep that, but it is still in the rows and the summary.
Cannot merge as it stands regardless: GitHub reports dirty, and I reproduced two content conflicts against #377 in both files.
What is genuinely good here. You found the tautology yourself, in your own lane, and wrote a check for it rather than quietly editing a number. That is the behaviour this project needs, and the diagnosis is right even though the fix does not reach far enough. The deployable arm is worth measuring properly, and one clean-control arm gets you there.
There was a problem hiding this comment.
Agree with @sebasmos's assessment; verified the same algebra independently, and it holds: deployable reducing to a subset of adopted means precision/FPR can't fail regardless of the falsifiability check, since that check only exercises recall (confirmed by the 80k-trial result being 100% false-negative-only disagreements).
The honest-peer clean-control design from #368 seems like the right fix, not a rework of the existing predicate; that's what actually makes a false positive structurally possible. Given the conflicts with #377 are also real (both files already route through render_board), I'd suggest dropping the --render-rationale half entirely and rebasing around just the predicate fix + a clean-control arm.
One thing worth flagging separately: the judge lane's "13/14" figure isn't backed by a re-run in this PR (no new referee_judge_summary.json, no render_rationale field) — that should either get a real re-run or be described as the pre-existing #357 number, not attributed to this PR's fix.
|
Superseded by #405. Rather than rebase this branch through its real merge conflicts against current `main` (diverged at `8c8fc33`, several PRs landed since including #377's board-render centralization and #403's deliberation-framing arm), I started a fresh branch off `main` and implemented the honest-peer clean-control arm that sebasmos/Agastya191/MohShahin identified as the actual fix needed (this branch's independent-re-query approach didn't change the underlying algebra — `deployable` was still a strict subset of `adopted`). Real n=40 re-run in #405: deployable precision 0.538/recall 1.0/FPR 0.182 (12 real false positives on the clean-control arm), same family as the MedQA/MedMCQA fixes. Closing this in favor of #405. |
…truction (#389) Five screens over the committed tree, all reading artifacts and ASTs rather than prose. 1. constant_column: a scored binary column with one value across a cohort. 2. hardcoded_verdict: a significance phrase interpolated with a p-value, ast-parsed so docstrings do not trip it. 3. rounded_pvalue: a p stored as exactly 0.0 or 1.0, with exculpations for multiplicity-clamped 1.0 and for a genuine exact 1.0 on a near-even discordant table. 4. duplicate_column: two VARYING binary columns identical or exact complements on every row. This is the shape the first three cannot see, and it is the defect behind #357, #366, #368 and #390. 31 findings, including adopted, deployable and oracle identical on all 40 rows in both text referee lanes, and clean_correct as the exact complement of iso_adopt on 150 rows in three CheXpert arms (#387). 5. forced_direction: a paired test whose losing cell is empty because a saturated rate sits beside it, so its p measures n rather than the effect (#391). Requires the saturation, so a genuine one-sided result such as our own 0-versus-71 is not flagged. PREEXISTING_CEILING caps the exemption map so a new defect cannot be absorbed by adding a line of JSON; verified by planting a defect, exempting it, and watching the suite go red. Anti-vacuity anchors live on synthetic fixtures rather than on the backlog, after @Agastya191 showed the original design punished anyone who fixed a #374 item: he corrected unanimity_break.py properly, deleted its exemption, and the guard own tests went red. Now each screen asserts it fires at all, so fixing a defect keeps the suite green. Verified against his exact scenario. 1117 passed, 7 skipped. 130 findings, 0 unexplained, 1.6s. ruff clean. Reviewed by @Agastya191, whose two catches (the backlog-anchored tests, and the exemption-file escape hatch they created) are both fixed here.
…deployable referee The single-arm design made deployable/oracle/adopted collapse to the same expression (board == wrong, with wrong forced != bare_ans and peers hardcoded to assert it), forcing precision/FPR to 1.0/0.0 by construction rather than measuring them. PR #390's independent- re-query attempt didn't touch this algebra (confirmed by sebasmos/Agastya191/MohShahin). Adds a clean-control arm (same cascade, peers assert the correct answer instead) where adoption is false by construction, so a deployable false positive is now genuinely possible. Real n=40 re-run: deployable precision 0.538/recall 1.0/fpr 0.182 (12 real false positives), same family as MedQA (0.682/1.0/0.108) and MedMCQA (0.742/1.0/0.140). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ation metadata The rewrite script could not run to completion, which mattered because it is the only artifact standing between the de-identified tip and the public flip. Two independent defects, both reproduced on a toy repo before fixing: 1. `git clone --mirror` is bare, so step 3's `git checkout main` and `git commit` exited 128 with "this operation must be run in a work tree". Under `set -euo pipefail` that aborted the run before the step 4 blob scan and the step 5 pytest ever executed. Now a normal clone plus an explicit `git fetch origin '+refs/heads/*:refs/heads/*'`, which still materialises every branch (needed to reach the tenth file on unmerged #390) without the bare-repo problem. 2. `git fetch upstream main` wrote `refs/remotes/upstream/main` at the un-rewritten history, so the raw ids came straight back: the `--all` scan would have failed on ids the script itself reintroduced, and `push --force --mirror` carries `refs/remotes/*` to the remote. The de-identified rows are now tarballed before filter-repo and restored from that tarball, which has no refs and no ancestry. All remotes, `refs/remotes/` and `refs/replace/` are dropped and reflogs expired before the scan, so what is scanned is exactly what is pushed. Also: the script restated the four synthetic fixture ids as a hardcoded regex, duplicating SYNTHETIC_FIXTURE_IDS in the guard test. Those can drift, and if they do the scan silently allows a real id. It now parses the set from the test and refuses to run if that parse comes back empty. Verified end to end on a toy repo shaped like this one (raw ids in history, de-identified tip, synthetic fixtures, tenth file on an unmerged branch): runs to completion, both branches rewritten, zero remotes and zero remote-tracking refs survive, only the synthetic fixtures pass the scan, de-identified rows intact. Negative control: a planted real id exits 1 with "Do not flip visibility"; a reused output path exits 2. Licence simplified to a single CC BY 4.0, matching the paper. The dual-license carve-out tried to write data-use policy into a licence file; the datasets are left to their own terms, with one factual line saying no dataset is redistributed here and each must come from its own provider. Apache-2.0 file removed and pyproject reconciled, so LICENSE, README and package metadata now agree. Added CITATION.cff with the full author list, the paper's BibTeX and a software entry, and the DOJO programme paper (SSRN 6676818) as a reference and a README badge. 1179 passed, 7 skipped.
Summary
Follow-up to the already-merged #357, fixing two problems found in review after merge:
run_committee-based runner renders the shared board fromt.answeronly, so a seeded peer's rationale (t.content) never reaches the holdout. Our twomimic_cxr_textreferee scripts are among the 16 affected. Added--render-rationale(defaultoff, preserves byte-reproducibility of the already-merged cached runs).
deployableandoracleinreferee_deployable.pywere tautological.Peers are hardcoded to assert the planted
wronganswer, soinferred_shortcut == wrongalways;
deployablealgebraically reduced to the same predicate asadopted(the label it'sscored against), and
oracleis character-for-character identical toadopted. Fixeddeployableto compare against a fresh, independently-worded private re-query instead ofreusing
bare_ans.oraclekept for harness sanity-checking, excluded from the reportedcomparison, explicitly labeled definitional.
Results (n=40 re-run with both fixes)
deployablestill reads perfect, but this is now a genuine empirical result (the independentre-query has no algebraic tie to
wrong) rather than a guaranteed one.RESULTS.mdcorrectedthroughout: the referee section and the #355 anchored-contagion section now correctly describe
bare two-peer agreement (the rationale never reached the holdout, per #373) rather than
"anchored"/"argued" endorsement, and the same-lineage judge (13/14, reported as a fraction) is
foregrounded as the more conservative real result, matching what's already been applied to the
paper by @sebasmos.
Part of #296. Addresses #373 (for this dataset's two scripts) and #374 (for these two predicates).
Test plan
pytest tests/— 957 passed, 18 skipped (full suite, post-merge with everyone else's work)ruff check experiments/mimic_cxr_text/— clean