refactor: overload the flag-driven return unions - #124
Merged
Conversation
Clears 22 pyrefly baseline entries across five files (135 -> 113). All 22 come from one pattern: a function whose return type is a union selected by a boolean argument, so every caller inherits both arms and has to answer for the one that cannot occur. `likelihood_numpy.golden_section_optimize_lambda_numpy` was already overloaded on `return_pab`; the helpers underneath it never were. Overloaded to match: - `_batch_reml_at_lambda_numpy` (`ndarray` vs `(ndarray, ndarray)`) - `_batch_reml_at_lambda_split_ncvt1_numpy` (same) - `_batch_golden_section_numpy` (2- vs 3-tuple, keyed on whether a Pab evaluator is passed; its two callables are now typed as `_BatchLoglFn` and `_BatchLoglPabFn` instead of bare untyped parameters) - `golden_section_optimize_lambda_split_ncvt1_numpy` (2- vs 3-tuple) `kinship.compute._stream_s_full_and_chr` returns `S_full` only when asked, so its `ndarray | None` first element forced five unguarded call sites in `compute_loco_kinship_streaming`. Overloaded on `S_full_accum`. `kinship.compute.compute_loco_kinship_streaming` itself returns either the iterator or an (iterator, cache) pair. Overloaded on `return_snp_stats`. That lets `lmm/loco.py` drop a `cast` whose comment already asserted the function "is overloaded on return_snp_stats" — it wasn't, until now — and clears the last entry in `pipeline.py`. `return_pab` and `S_full_accum` are now keyword-only, which is what makes two overloads express the signature exactly rather than three. Every existing call site already passed them by keyword, so no call changes. The one non-signature change: `compute_loco_kinship_streaming` now returns on `if snp_stats_cache is not None` rather than `if return_snp_stats`. The two are equivalent by construction, and the cache stays built at its original point because `SnpStats.__post_init__` marks the stats arrays read-only in place, so moving it would change when that happens. Nothing here alters runtime behaviour: `@overload` stubs are erased, `cast` is the identity, and the narrowing swap is equivalent. Checked rather than asserted — the LOCO streaming path (the one restructured) produces an identical .assoc.txt, identical pve/pve_se, identical per-chromosome kinship sums, and an identical stats cache down to the read-only array flags. `pytest tests/` — 2241 passed, 10 skipped, 3 xfailed. `pytest -m "tier2 or slow"` — 38 passed. Baseline regenerated on Python 3.11 / numpy 2.4.6 per #121; `pyrefly check --baseline` reports 0 errors on both 3.11.13/2.4.6 and 3.13.5/2.5.1.
This was referenced Jul 26, 2026
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
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.
Continues the pyrefly burn-down. 135 → 113 entries.
All 22 entries come from one pattern: a function whose return type is a union selected
by a boolean argument. Every caller inherits both arms and has to answer for the one that
cannot occur — so a single un-overloaded callee scatters errors across the files that use it.
kinship/compute.py_stream_s_full_and_chr(5),compute_loco_kinship_streaming(1)lmm/likelihood_numpy.pylmm/compute_numpy.pygolden_section_optimize_lambda_split_ncvt1_numpypipeline.pycompute_loco_kinship_streaminglmm/loco.pylikelihood_numpy.py— finishing a started migrationgolden_section_optimize_lambda_numpywas already overloaded onreturn_pab. Thehelpers beneath it never were, so the union just moved one level down. Overloaded to match:
_batch_reml_at_lambda_numpy—ndarrayvs(ndarray, ndarray)_batch_reml_at_lambda_split_ncvt1_numpy— same_batch_golden_section_numpy— 2- vs 3-tuple, keyed on whether a Pab evaluator is passed.Its two callable parameters were bare and untyped; they now have named aliases
(
_BatchLoglFn,_BatchLoglPabFn).golden_section_optimize_lambda_split_ncvt1_numpy— 2- vs 3-tuplekinship/compute.py_stream_s_full_and_chrreturnsS_fullonly when asked, so itsndarray | Nonefirstelement forced five unguarded call sites inside
compute_loco_kinship_streaming.compute_loco_kinship_streamingitself returns either the iterator or an(iterator, cache)pair. Overloading it letslmm/loco.pydrop this:The comment asserted an overload that did not exist. Now it does, so the
castis gone andthe claim is enforced by the type checker instead of by a comment.
Signature changes
return_pabandS_full_accumare now keyword-only. That is what lets two overloadsexpress the signature exactly instead of needing three (a
Literal[True]arm can't follow adefaulted parameter positionally). Every existing call site — production and tests — already
passed them by keyword, so no call site changed. Verified by grep across the repo.
The one non-signature change
compute_loco_kinship_streamingnow returns onif snp_stats_cache is not None:rather thanif return_snp_stats:. Equivalent by construction, and it narrows where the flag doesn't.I first tried moving the cache construction into the return branch — that was wrong, for
two reasons worth recording: there is a deliberate
if not return_snp_stats: del statsthatfrees the stats early, and
SnpStats.__post_init__marks the stats arrays read-only inplace, so relocating the construction would change when that happens. The cache is built
where it always was.
Verification
@overloadstubs are erased at runtime,castis the identity, and the narrowing swap isequivalent — so no behaviour change is possible here. Checked anyway, on the LOCO streaming
path this actually restructures:
.assoc.txtpve,pve_se,n_snps_testedSnpStatsCache—col_means/col_vars/miss_countssums,n_snps,sample_scope, andcol_means.flags.writeablepytest tests/pytest -m "tier2 or slow"ruff check/ruff format --checkThe
tier2 or slowrun is included because the default addopts filter skips it and itcovers the streaming path this touches.
Baseline
Regenerated on Python 3.11.13 / numpy 2.4.6 per #121, verified on both:
pyrefly check --baselineRunning total
174 (#121) → 152 (#122) → 135 (#123) → 113.
pipeline.py,validation/compare.py,kinship/compute.pyandlmm/likelihood_numpy.pyare now at zero.Next largest:
tests/test_incremental_writer.py(23, undiagnosed),tests/test_jlinalg_dgemm.py(8),
lmm/io.py(7),lmm/compute_numpy.py(6 — Optional narrowing across a mode dispatch,plus
TypedDict.update).