refactor: model the assoc test type as an enum and two tables - #152
Merged
Conversation
compare_assoc_results derived three mutually exclusive booleans from one sample of the reference rows, with Wald as the implicit else of an if/elif chain. That is an enum spelled as a boolean triple, and the four cases were re-derived in three separate places. #122 merged two of those places and its comment claimed they shared one dispatch. They did not. The SNP-count-mismatch early return still asked `is_score_test or is_all_tests` twice to decide which optional slots to populate, and each of the four branches still ended in its own conjunction of `.passed` attributes, four overlapping copies of the same rule sitting directly below the column selection they had to agree with. AssocTestType plus _detect_assoc_test_type replaces the triple. _OPTIONAL_SLOTS says which optional result slots a type populates, and the early return reads it instead of re-deriving. _PASS_COLUMNS says which columns the verdict reads, so the four conjunctions become one all(). LRT's NaN beta/se rule is one named branch rather than an inline special case buried in a 240-line function. The per-type column selection stays a four-way branch. Those branches call different helpers with different tolerances and skip messages, so there is nothing there to collapse, and pretending otherwise would hide real differences behind a table. The file grows 639 -> 668 lines. The enum, the detector and the two tables cost more lines than the conjunctions they replace. What improves is that the case analysis is in three named places instead of re-derived inline in three others, and the verdict is one line to check rather than four conjunctions of five to ten terms. CODEMAP's six compare.py line anchors are corrected; the #145 gate caught them. Verified with a pin that runs compare_assoc_results over all seven committed .assoc.txt fixtures, covering all four detected test types, in four conditions each (identical, one row short, one SNP renamed, beta perturbed past tolerance). It digests the fully rendered AssocComparisonResult, skip messages included. All 28 digests are identical to master. scripts/demonstrate_equivalence.py - VERDICT: ALL FIELDS PASS TOLERANCES. pytest tests/ - 2278 passed, 7 skipped, 3 xfailed. pyrefly check - 0 errors, 43 suppressed, unchanged from master.
Both branches added entries under Unreleased. Kept all, with master's first.
michael-denyer
added a commit
that referenced
this pull request
Jul 28, 2026
…sences (#154) Two small things, neither changing behaviour. check-doc-anchors.py has to work out which symbol an anchor means, and for CODEMAP's tables that is a guess. Those rows label the link file.py:123 and put the symbol in a separate column, so _wanted_symbol takes the first plausible backticked name in the row. A row naming two symbols can be checked against the wrong one, which passes when that other symbol happens to sit on the anchored line. That was documented nowhere a reader would hit it. _wanted_symbol's docstring explained its fallback mechanism without saying the choice is a guess, and DEVELOPMENT.md named the hook among the prek gates and said nothing about its guarantee. The gate now sits under 94 anchors and caught real rot during the 7.2.0 doc work, so its reputation was running ahead of what it can prove. Both places say so now: a green run means no anchor is provably wrong, not that every anchor is provably right. The fix is to label each link with its own symbol, at which point the label is authoritative and the row is never consulted. That is roughly 120 links and is not done here; the limitation is recorded instead of quietly relied on. load_gemma_assoc read optional columns through _opt_float and then, on the next two lines, beta and se through an inline float(row.get(name, "nan")). Two idioms sat side by side for what looked like one idea. It is not one idea: an absent optional column is None because the test does not report it, and an absent beta is NaN because AssocResult requires it and GEMMA's LRT formats omit it. Both now go through a named helper, _opt_float and _float_or_nan, so the difference is stated rather than inferred from which idiom the author reached for. The helper cost 12 lines, which shifted three CODEMAP anchors into compare.py. The gate caught them; corrected here. Verified with the same pin as #152: compare_assoc_results over all seven committed .assoc.txt fixtures, four conditions each, digesting the rendered AssocComparisonResult. All 28 digests identical to master. pytest tests/ - 2275 passed, 7 skipped, 3 xfailed. prek run --all-files - all hooks pass, check-doc-anchors at 94 OK.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
compare_assoc_resultsderived three mutually exclusive booleans from one sample of the reference rows, with Wald as the implicitelseof an if/elif chain. That is an enum spelled as a boolean triple, and the four cases were re-derived in three separate places.#122 merged two of those places, and its comment claimed they shared one dispatch. They did not. The SNP-count-mismatch early return still asked
is_score_test or is_all_teststwice to pick which optional slots to populate, and each of the four branches still ended in its own conjunction of.passedattributes: four overlapping copies of the same rule, sitting directly below the column selection they had to agree with.What changed
AssocTestTypeplus_detect_assoc_test_typereplaces the triple._OPTIONAL_SLOTSsays which optional result slots a type populates, and the early return reads it instead of re-deriving the conditions._PASS_COLUMNSsays which columns the verdict reads, so the four conjunctions become oneall(...). LRT's NaN beta/se rule is one named branch rather than an inline special case buried in a 240-line function.The per-type column selection stays a four-way branch. Those branches call different comparison helpers with different tolerances and different skip messages, so there is nothing there to collapse; folding them into a table would hide real differences.
The file gets longer, and that is the trade
639 to 668 lines. The enum, the detector and the two tables cost more lines than the conjunctions they replace. What improves is that the four-way case analysis lives in three named places instead of being re-derived inline in three others, and the verdict is one line to check rather than four conjunctions of five to ten terms each. If you would rather not pay 29 lines for that, this is the PR to push back on.
Verification
A pin runs
compare_assoc_resultsover all seven committed.assoc.txtfixtures, which between them cover all four detected test types, in four conditions each: identical, one row short (the early return), one SNP renamed (the ID-mismatch path), and beta perturbed pastbeta_rtol(a failing column). It digests the fully renderedAssocComparisonResult, skip messages included, so a changed message shows up as a changed digest. All 28 digests are identical to master.scripts/demonstrate_equivalence.py: VERDICT: ALL FIELDS PASS TOLERANCESpytest tests/: 2278 passed, 7 skipped, 3 xfailedpyrefly check: 0 errors, 43 suppressed, unchanged from masterprek run --all-files: all hooks passCODEMAP's six
compare.pyline anchors are corrected in the same commit. The #145 gate caught them, which is the first time it has earned its keep on a change that was not about docs.