Skip to content

refactor: clear the 22 pyrefly baseline entries in validation/compare.py - #122

Merged
michael-denyer merged 1 commit into
masterfrom
chore/pyrefly-compare
Jul 26, 2026
Merged

refactor: clear the 22 pyrefly baseline entries in validation/compare.py#122
michael-denyer merged 1 commit into
masterfrom
chore/pyrefly-compare

Conversation

@michael-denyer

Copy link
Copy Markdown
Owner

Continues the pyrefly burn-down from #118 / #121. 174 → 152 entries.

Two root causes, both in src/jamma/validation/compare.py.

1. The dict-splat — 15 errors at one call site

load_gemma_assoc built a row dict through _parse_assoc_field, which returns
str | int | float, then splatted it: AssocResult(**row). That union reaches every
parameter, so all 15 fields error at the single line 299.

Replaced with explicit construction: raw cells stay dict[str, str], and each field gets
its own int() / float(). Six float | None columns go through a new _opt_float(row, name)
helper that returns None when the layout omits the column. Header names already equal
AssocResult field names, so the mapping is direct — verified, not assumed: every column
across all seven accepted layouts is an AssocResult field, and every field appears in at
least one layout.

_parse_assoc_field, _ASSOC_STR_FIELDS and _ASSOC_INT_FIELDS had no other callers and
are deleted.

2. The duplicated dispatch — 7 errors

compare_assoc_results ran the same four-way test-type dispatch twice: once to compute the
per-column ComparisonResults, once to fold them into passed. The second chain read
p_score_result.passed, p_lrt_result.passed and l_mle_result.passed with no narrowing,
because the assignments happened in the first chain — 6 missing-attribute errors. The 7th
was passed=all_passed, which picked up numpy.bool from the LRT branch's np.all(...).

Merged into one dispatch so each branch computes its columns and its verdict together, and
cast the two np.all results to bool.

That also removes a latent AttributeError. The two chains ordered the branches differently
— first all / score / lrt / wald, second all / lrt / score / wald. is_score_test and
is_lrt_test are not structurally exclusive (both require not is_all_tests and all
p_wald is None), so a result set satisfying both would compute the Score columns and then
read p_lrt_result.passed on None. Unreachable from load_gemma_assoc — no header layout
carries p_score and p_lrt without p_wald — but reachable through the public API, which
takes a list[AssocResult] directly. The merge makes it structurally impossible.

Verification

Equivalence proved against the fixtures rather than argued:

Check Result
Old vs new parser over all 19 .assoc.txt fixtures identical — 90,644 rows, exact repr match (catches 1 vs 1.0 type drift, not just value drift)
Old vs new compare_assoc_results identical — 95 cases: identity, three perturbation scales, and a length mismatch, per fixture
pytest tests/ 2241 passed, 10 skipped, 3 xfailed
ruff check / ruff format --check clean

Fixture coverage spans all four LMM modes (wald, lrt, score, all) and all seven header
layouts, including both logl_H1-present and logl_H1-absent variants.

Baseline

Regenerated on Python 3.11.13 / numpy 2.4.6 — the requires-python floor, per #121
and verified on both interpreters:

Environment pyrefly check --baseline
Python 3.11.13, numpy 2.4.6 0 errors
Python 3.13.5, numpy 2.5.1 0 errors

Real backlog without --baseline: 152, down from 174.

Note for anyone repeating this: a fresh uv sync in a new worktree resolves 3.13, not
the floor — there is no .python-version in the repo. uv sync -p 3.11 first, or the #121
regression comes straight back.

Two root causes, both in `validation/compare.py`.

`load_gemma_assoc` built a `dict[str, str | int | float]` through
`_parse_assoc_field` and splatted it into `AssocResult(**row)`. The union
reaches every parameter, so all 15 fields error at the one call site.
Constructs `AssocResult` explicitly instead, with per-field `int()`/`float()`
and an `_opt_float` helper for the six `float | None` columns. Header names
already equal `AssocResult` field names, so the mapping is direct.
`_parse_assoc_field`, `_ASSOC_STR_FIELDS` and `_ASSOC_INT_FIELDS` had no other
callers and are gone.

`compare_assoc_results` ran the same four-way test-type dispatch twice: once to
compute the per-column results, once to combine them into `passed`. The second
chain read `p_score_result.passed`, `p_lrt_result.passed` and
`l_mle_result.passed` with no narrowing, since the assignment happened in the
first chain. Merged into one dispatch, so each branch computes its columns and
its verdict together. That also removes a latent `AttributeError`: the two
chains ordered `is_score_test` and `is_lrt_test` differently, so a result set
satisfying both predicates would compute the Score columns and then read the
LRT ones. Also casts the LRT `np.all(...)` results to `bool` for `passed`.

Verified equivalent rather than assumed: parsing all 19 `.assoc.txt` fixtures
(90,644 rows, all seven header layouts) with the old and new parsers gives
byte-identical `AssocResult` reprs, and `compare_assoc_results` returns
identical results over 95 cases (identity, three perturbation scales, and a
length mismatch, per fixture).

Baseline regenerated on Python 3.11 / numpy 2.4.6 per #121: 174 -> 152 entries.
`pyrefly check --baseline` reports 0 errors on both 3.11.13/numpy 2.4.6 and
3.13.5/numpy 2.5.1. `pytest tests/` — 2241 passed, 10 skipped, 3 xfailed.
@michael-denyer
michael-denyer merged commit 9ae5a1b into master Jul 26, 2026
12 checks passed
@michael-denyer
michael-denyer deleted the chore/pyrefly-compare branch July 26, 2026 22:59
michael-denyer added a commit that referenced this pull request Jul 28, 2026
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.
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.

1 participant