test: mechanical screen for reported metrics that cannot fail by construction (#374) - #389
Conversation
Agastya191
left a comment
Agastya191
left a comment
There was a problem hiding this comment.
good work on degeneracy.py and the exemptions file, and the sabotage fixtures are the right way to show a screen is not vacuous. One problem is that in test_degeneracy_guard.py the anti-vacuity checks are anchored to the backlog they exist to clear: all 12 keys in test_the_screen_finds_the_known_374_instances are also preexisting entries, and test_all_three_screens_fire_on_the_real_tree asserts hardcoded_verdicts >= 2 against a tree that has exactly 2. I fixed unanimity_break.py the way #374 asks, computing the verdict from mc.pvalue instead of hardcoding it, and deleted its preexisting entry, and the suite went red on both of those tests rather than on anything new, so the first person to clear a #374 item has to edit the guard's own tests to get green and will reasonably reach for the exemption file instead. You have multiple options to fix this, but I would move the anchors onto synthetic fixtures that nobody will ever fix, and assert each screen fires at all rather than pinning a count that only holds while the defects are still in the tree.
|
You are right, and this was the good kind of review catch: not a bug in a screen but a bug in the incentive the tests create. Fixed rather than argued. The anchors were 12 real keys from the #374 table and the yield floors were pinned to the tree's exact counts, so the tests were anchored to the backlog they exist to clear. Your demonstration is the proof: fix an item properly, delete its exemption, and the guard's own tests go red while nothing new is wrong. Anyone hitting that will reach for the exemption file, which is precisely the behaviour this PR is supposed to discourage. Done exactly as you suggested. The anchors now sit on synthetic fixtures nobody will ever clean up, one per screen, and both that test and the real-tree check assert each screen fires at all rather than pinning a count. I verified it against your scenario rather than assuming: derived the Your review also made me spot two tests of my own with the identical fault, both now gone: Two things landed since you looked, both from the same review round, so worth a re-read of the diff rather than the branch you saw:
Both use machinery that was already in the file, which is a bit galling in hindsight: |
…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.
…eree_deployable.py conflict Combines #377's board-render fix (render_board, so a peer's rationale can reach the holdout) with this branch's own clean-cascade control (a second, honest-peer arm so deployable no longer collapses to the adoption label it is scored against). Both touch the same _Hold class; verified render_board's default output (show_rationale=False) is byte-identical to the old inline board join, so the committed MedMCQA cache still replays this exact code path. The #389 degeneracy guard, scanning MedMCQA for the first time, flagged 27 findings. All independently re-verified: 20 rounded p-values are round(p, 6) underflowing genuinely tiny exact McNemar values (checked against benchmaxxing.stats.mcnemar directly, none are computation bugs); naive is constant True because the naive gate cannot separate honest agreement from adoption in either of this branch's cascade arms, exactly the paper's own claim about it; oracle_audit duplicates adopted by the script's own documented design ('upper-bound only'), distinct from the pre-#368 defect where deployable also duplicated it. Three findings (control_adopt, isolated_adopt, wrong_orch_output_wrong, neither_adopt) resolve open human-call items already sitting in the MedQA half of the exemptions file; migrated those out of preexisting into allowlist with the verified reason. Three stale MedQA exemption keys (contrasts.1/3/8) were removed: #368's own capstone regeneration already fixed or dropped those contrasts. All 1117 tests pass.
…rarchy, controls, referee + corrected family (#368) Rebased onto current main and the referee_deployable.py conflict against #377 resolved by hand: combined #377's render_board fix with this branch's own clean-cascade control (verified byte-identical default output, so the committed cache still replays). Items A-G from my change request addressed: board renderer routed through render_board, Holm count corrected to 4/8, oracle/deployable tautology labeled and fixed with a clean-control arm, recall-1.0 forced-ness noted, manifest question deferred (tracked separately). Item H (clean_a's control) filed as #394, not blocking. The #389 degeneracy guard flagged 27 findings on this cohort scanned for the first time; all independently re-verified against benchmaxxing.stats.mcnemar and the underlying scripts, none are computation bugs, all now recorded in tests/degeneracy_exemptions.json with the specific reason. All 1117 tests pass.
… that survives The #389 guard has a ratchet that fails when an exemption no longer matches any finding, so that stale lines cannot accumulate and hide the next real defect. Fixing the tautology trips it, which is the intended behaviour rather than a problem with this PR: duplicate_column|.../referee_deployable.jsonl|adopted vs deployable deleted duplicate_column|.../referee_deployable.jsonl|deployable vs oracle deleted Both described the identity this PR breaks. adopted vs oracle stays, since oracle_audit is still definitional by design, and its justification is rewritten to record that deployable is no longer a duplicate: it differs from the label on 12 of 80 rows and gives a measured 0.538/1.0/0.182. 1145 passed, 7 skipped.
2 participants
Closes nothing on its own. Adds the standing check #374 asks for, so this class of defect stops reaching the paper one PR at a time.
benchmaxxing/degeneracy.pyscreens the committed tree;tests/test_degeneracy_guard.pyfails when a screen fires on something with no written exemption. Standalone report:python -m benchmaxxing.degeneracy.The three screens
experiments/*/results/**(all results.jsonl, plus thedeid/*.csvexports) is read row by row. Any column whose values all lie in {0, 1, true, false} and never varies is reported with file, column, constant value and row count. A per-case file is identified by every record carrying a case id, which is what keeps the keyed call caches and the per-turn transcripts out of scope, rather than a filename blocklist that would rot.experiments/is parsed withast, not grepped. A verdict literal is reported only when it sits in the same f-string,.formatcall or%-format as an interpolated p-value. Implicit concatenation is handled, which is what the realunanimity_break.pycase needs since the verdict is in one fragment andp=in the next.Green today, with a two-map exemption file, and why
It passes today. The 84 findings below are all pre-existing; none is introduced by this PR. Failing the suite on them would mean either landing a red
mainor bundling 84 artifact fixes into the same PR as the guard, and both make the guard harder to review than the backlog it exposes.Rather than one allowlist doing two different jobs,
tests/degeneracy_exemptions.jsonhas two maps:allowlist(8 entries). Verified legitimate. Each reason states the table that was checked. Every one is a p-value of exactly 1.0 that a hand-computed exact test really does return on a non-degenerate table, for examplenet_harm.jsoncorner_tagwhere harm is 21/22 and rescue 13/13, so the table is [[21,1],[13,0]], only two tables are possible at those margins, andscipy.stats.fisher_exactreturns exactly 1.0. Thecablecue in the same file is 22/22 vs 13/13, an empty column, and is deliberately left flagged.preexisting(76 entries). What was already in the tree. Not an endorsement of any of it. It exists so the guard can be switched on while the backlog is worked, and so any new instance fails immediately.Both maps require a non-empty reason (a 40-character floor, so
n/adoes not buy an exemption), keys are exact rather than globs, and an entry that no longer matches a finding fails the suite. Deleting the line is the last step of a fix, so an exemption cannot outlive the defect it names.Nothing in the constant-column or hardcoded-verdict screens was allowlisted. I could not establish that any of those 26 is legitimate, so none is claimed to be.
Evidence that it is not vacuous
#374 notes that earlier guards passed when a comment satisfied them. These screens read artifacts and parse ASTs, and the tests demonstrate the behaviour rather than asserting it:
experiments/medqa/results/SABOTAGE_probe.jsonlwith 60 rows, one planted constant column and one random column,git added so it is tracked. The guard went red:1 unexplained,constant_column|...|planted_control_adopt (binary column constant at False across all 60 rows), andtest_no_unexplained_cannot_fail_metricsfailed. The random column was not flagged. Probe removed.test_no_stale_exemptions. A reason of"n/a"failedtest_every_exemption_carries_a_written_reason.ref_flag,naive_flag,shared_adopt,clean_correct,op0.15_shared_adopt,adopted,deployable,anchored).'significant' if p < 0.05 else 'not significant'is not flagged.tests/test_degeneracy_guard.py46 tests in 4.2s; whole suite 1095 passed, 7 skipped, 11.1s. Comfortably inside the normal suite.What it cannot catch
Stated plainly so a green run is not read as an all-clear. Two of these are asserted as executable tests so the holes stay on the record:
deployableinreferee_deployable.py, equalsadoptedon all 40 rows and both vary. Column-wise constancy cannot see that, nor a predicate that reduces to the label through an intermediate variable. Only thenaivecomparator in that file is caught, because it happens to be constant.experiments/support2/results/holds only the manifest and provenance), so nothing here reports them. The knownstats_reconciliation_summary.jsonfields are all four caught.Duplicate-column equality and a near-constant threshold are the obvious next screens. Both need a per-arm judgement about which saturation is legitimate, which is why they are not in this PR.
What it flags today
Constant binary columns (24)
experiments/blind_metric/results/blind_metric.jsonlbase_is_decoyexperiments/chexpert/results/imaging_blind_metric.jsonlbase_is_decoyexperiments/chexpert/results/imaging_blind_metric.jsonlnamed_rubric_when_driftedexperiments/imaging/results/imaging_blind_metric.jsonlbase_is_decoyexperiments/imaging/results/imaging_blind_metric.jsonlnamed_rubric_when_driftedexperiments/imaging/results/imaging_cascade_cable.jsonlshared_adoptexperiments/imaging/results/imaging_peer_size_curve.jsonlk4_adoptexperiments/imaging/results/imaging_strength_cascade.jsonlop0.15_shared_adoptexperiments/imaging/results/imaging_strength_cascade.jsonlop0.3_shared_adoptexperiments/imaging/results/imaging_strength_cascade.jsonlop0.45_shared_adoptexperiments/imaging_chexpert/results/device_absent/imaging_cascade_none.jsonlplacebo_adoptexperiments/imaging_chexpert/results/full_runs/imaging_cascade.jsonlplacebo_adoptexperiments/imaging_chexpert/results/system_flag/imaging_system_flag.jsonliso_adoptexperiments/medqa/results/authority_ladder.jsonlcontrol_adoptexperiments/medqa/results/majority_pressure.jsonlisolated_adoptexperiments/medqa/results/orchestrator_failure.jsonlwrong_orch_output_wrongexperiments/medqa/results/super_additivity.jsonlneither_adoptexperiments/mimic_cxr_image/results/deid/blind_metric.csvbase_is_decoyexperiments/mimic_cxr_image/results/deid/strength_cascade.csvop0.3_shared_adoptexperiments/mimic_cxr_text/results/blind_metric.jsonlbase_is_decoyexperiments/mimic_cxr_text/results/blind_metric.jsonlnamed_rubric_when_driftedexperiments/mimic_cxr_text/results/break_it_a_per_case.jsonlcontrolexperiments/mimic_cxr_text/results/referee_deployable.jsonlnaiveexperiments/referee/results/referee_deployable.jsonlnaiveHardcoded significance verdicts (2)
experiments/medqa/majority_pressure.pymain:not significantexperiments/medqa/unanimity_break.pymain:NOT significantp-values committed as exactly 0.0 or 1.0 (58: 8 allowlisted, 50 pre-existing)
experiments/cross_dataset/results/medqa_vs_medmcqa.jsonper_cue.option_order.fisher.pvalueexperiments/family_correction/results/family_correction.jsonrows.10.p_rawexperiments/family_correction/results/family_correction.jsonrows.11.p_rawexperiments/family_correction/results/family_correction.jsonrows.12.p_rawexperiments/family_correction/results/family_correction.jsonrows.4.p_rawexperiments/imaging/results/net_harm.jsonper_cue.corner_tag.harm_vs_rescue_fisher.pvalueexperiments/imaging/results/net_harm.jsonper_cue.laterality.harm_vs_rescue_fisher.pvalueexperiments/imaging/results/net_harm.jsonper_cue.watermark.harm_vs_rescue_fisher.pvalueexperiments/family_correction/results/family_correction.jsonrows.9.p_rawexperiments/imaging/results/imaging_cue_combo_summary.jsonboth_vs_stronger_single.pvalueexperiments/imaging/results/imaging_majority_pressure_summary.jsonone_vs_two_peer_mcnemar.pvalueexperiments/imaging/results/imaging_peer_size_curve_summary.jsonone_vs_two_mcnemar.pvalueexperiments/imaging/results/net_harm.jsonper_cue.cable.harm_vs_rescue_fisher.pvalueexperiments/imaging_chexpert/results/natural_independent/confirmatory_e1.jsonfisher_pvalueexperiments/imaging_chexpert/results/natural_independent/holm_bonferroni.jsonresults.E1.p_rawexperiments/medqa/results/attributed_tier_summary.jsonjunior_model_vs_senior_model.pvalueexperiments/medqa/results/attributed_tier_summary.jsonunlabeled_vs_junior_model.pvalueexperiments/medqa/results/authority_ladder_summary.jsonadjacent_rung_mcnemar.automated_system_vs_clinical_guideline.pvalueexperiments/medqa/results/authority_ladder_summary.jsonadjacent_rung_mcnemar.colleague_vs_senior_attending.pvalueexperiments/medqa/results/committee_size_sweep_summary.jsons0_vs_s1.pvalueexperiments/medqa/results/committee_size_sweep_summary.jsons0_vs_s2.pvalueexperiments/medqa/results/committee_size_sweep_summary.jsons0_vs_s4.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.0.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.1.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.10.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.11.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.12.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.2.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.21.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.22.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.23.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.3.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.4.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.5.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.6.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.7.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.8.pvalueexperiments/medqa/results/cross_lane_reconciliation_summary.jsontests.9.pvalueexperiments/medqa/results/deliberation_framing_summary.jsonnone_vs_critical.pvalueexperiments/medqa/results/deliberation_framing_summary.jsonnone_vs_independent.pvalueexperiments/medqa/results/dose_response_summary.jsonfaint_vs_assert.pvalueexperiments/medqa/results/dose_response_summary.jsonfaint_vs_emphatic.pvalueexperiments/medqa/results/dose_response_summary.jsonlean_vs_emphatic.pvalueexperiments/medqa/results/leader_as_auditor_summary.jsonauditor_vs_signoff.pvalueexperiments/medqa/results/leader_as_auditor_summary.jsonpeer_vs_auditor.pvalueexperiments/medqa/results/leader_as_auditor_summary.jsonpeer_vs_signoff.pvalueexperiments/medqa/results/pre_emptive_referee_summary.jsonno_vs_soft.pvalueexperiments/medqa/results/rationale_validity_summary.jsonbare_vs_named_fallacy.pvalueexperiments/medqa/results/rationale_validity_summary.jsonbare_vs_valid_wrong.pvalueexperiments/medqa/results/seed_confidence_summary.jsonconfident_vs_hedged_mcnemar.pvalueexperiments/medqa/results/seed_timing_summary.jsonlast_vs_first.pvalueexperiments/medqa/results/stats_reconciliation_summary.jsoncontrasts.0.pvalueexperiments/medqa/results/stats_reconciliation_summary.jsoncontrasts.1.pvalueexperiments/medqa/results/stats_reconciliation_summary.jsoncontrasts.3.pvalueexperiments/medqa/results/stats_reconciliation_summary.jsoncontrasts.8.pvalueexperiments/medqa/results/test_awareness_summary.jsonneutral_vs_agreement_eval.pvalueexperiments/medqa/results/text_cue_types_summary.jsonbaseline_vs_negation.pvalueexperiments/mimic_cxr_image/results/imaging_system_flag_summary.jsonshared_vs_isolated_mcnemar.pvalueRefs #374. No reviewers requested; assigning is yours.