Skip to content

Speed up the test suite without dropping coverage - #1011

Merged
aarmey merged 5 commits into
masterfrom
worktree-memoized-questing-deer
Sep 12, 2026
Merged

aarmey merged 5 commits into
masterfrom
worktree-memoized-questing-deer

Conversation

@aarmey

@aarmey aarmey commented Sep 12, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Correctness fix: stateCommon.py grabbed the wrong Cython fused-function overload for the digamma function (psi) — __pyx_fuse_0psi is psi's complex128 specialization, not the real-valued one (__pyx_fuse_1psi). Calling the complex overload through a real-valued ctypes.CFUNCTYPE reads garbage for the argument's missing imaginary half. This silently biased gamma_mle_closed_form's shape-parameter MLE (its "exact, instantaneous" uncensored fast path — e.g. a synthetic Gamma(4, 2) sample recovered shape 3.84 instead of 3.98) and threw off gamma_estimator's analytic gradient by ~5-10x in the shape direction for censored fits. That bad gradient is what was causing SLSQP to hit its 200-iteration cap on otherwise well-posed problems and fall back to the much slower trust-constr solver, which was the dominant remaining cost in test_cv and test_works_through_crossval. Root-caused by comparing the JIT-compiled function's output against its own .py_func (pure-Python) fallback, which disagreed — see commit for the full trace.
  • Rewrote StateDistribution.division_probability()'s integrand to use scipy.special (gammaln/gammaincc) directly instead of scipy.stats frozen-distribution pdf/sf, which was ~100x more expensive per call inside the quad() integration used every M-step.
  • Rewrote StateDistribution.logpdf() the same way — it runs once per state on every E step over the whole lineage array, and was still going through the frozen rv_continuous machinery. Verified the new output matches the old to float precision. Cuts test_works_through_crossval from ~14.7s to ~6.3s in isolation on top of the division_probability fix.
  • Enabled pytest-xdist (-n auto, already a dev dependency but unused) via addopts in pyproject.toml, and pinned OMP_NUM_THREADS/OPENBLAS_NUM_THREADS/MKL_NUM_THREADS/NUMEXPR_NUM_THREADS to 1 in a new lineage/tests/conftest.py so xdist workers don't fight scipy's own internal BLAS threading for cores.
  • Combined, the full suite drops from ~125s to ~16s wall clock, with no tests removed, skipped, or weakened — and the shape-parameter MLE is now unbiased.

Remaining known costs (not addressed here)

  • Numba JIT compilation of gamma_estimator's njit functions pays a one-time ~3-4s cost per process the first time each type signature is hit; under -n auto several workers re-pay it independently. Would need cache=True on the njit decorators to amortize across processes/runs.
  • The Baum-Welch E-step recursion (HMM/E_step.py:get_beta_and_NF) is plain Python/numpy, not JIT-accelerated, and is now one of the larger remaining per-test costs (~7s of test_cv[0]'s ~12.5s in isolation). This is legitimate algorithmic cost rather than a bug, and a bigger undertaking to address than the leaf-function fixes above.

Test plan

  • uv run pytest — all tests pass (67, including master's new Hypothesis tests) in ~16s, down from ~125s on the original branch point
  • uv run ruff check on touched files — passes
  • Verified the psi fix numerically: compared the JIT function against its .py_func pure-Python twin on identical inputs (they now agree; they didn't before), and checked __pyx_fuse_1psi against scipy.special.psi directly across several values
  • Verified logpdf()'s new output matches the old frozen-distribution implementation to float precision (np.testing.assert_allclose(..., rtol=1e-10, atol=1e-10)) on synthetic data including censored/unknown-fate rows
  • Spot-checked division_probability() still matches the existing test_sub_densities_sum_to_one / test_estimator_recovers_parameters / test_phase_estimator_recovers_parameters correctness checks

🤖 Generated with Claude Code

aarmey and others added 5 commits September 12, 2026 08:06
The two slowest tests spent most of their time in
StateDistribution.division_probability(), which numerically integrates a
product of scipy.stats frozen-distribution pdf/sf calls; that generic
rv_continuous machinery costs far more than the arithmetic itself when
called ~200 times per quad() evaluation. Rewriting the integrand against
scipy.special (gammaln/gammaincc) directly cuts
test_works_through_crossval from ~45s to ~18s with identical results.

Also enable pytest-xdist (-n auto, already a dev dependency but unused)
and pin BLAS/OpenMP thread counts to 1 in a new tests/conftest.py so
xdist's worker processes don't fight scipy's own internal threading for
cores. Combined, the full suite drops from ~125s to ~47s.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
lineage/states/stateCommon.py grabbed the raw address of
scipy.special.cython_special.__pyx_fuse_0psi and called it through a
ctypes CFUNCTYPE(c_double, c_double). That symbol is psi's *complex128*
specialization ("__pyx_t_double_complex (__pyx_t_double_complex, int)");
the real-valued one used here is __pyx_fuse_1psi. Calling the complex
overload through a real signature reads garbage for the missing
imaginary half of the argument. It happened to look right under plain
ctypes calls (the unused slot came back zero by luck), which is why this
went unnoticed, but numba's own generated call sequence for the same
function pointer leaves that slot non-zero, so every njit call to psi()
silently returned a wrong value.

This directly biased two things:
- gamma_mle_closed_form's Newton-Raphson solve for the shape parameter
  (its "100% exact, instantaneous" uncensored fast path), e.g. a
  synthetic Gamma(4, 2) sample recovered shape 3.84 instead of 3.98.
- gamma_estimator's analytic gradient for uncensored observations,
  which was off by roughly 5-10x in the shape-parameter direction. That
  bad gradient is why SLSQP kept hitting its 200-iteration cap on
  otherwise well-posed censored fits and falling back to the much
  slower trust-constr solver, which was the dominant cost in
  test_cv[3] and test_works_through_crossval.

Fixing the symbol lookup resolves both: the full test suite drops from
~47s to ~22s on top of the earlier speedups, and the shape MLE is now
unbiased.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
logpdf() runs once per state on every E step over the whole lineage
array, but still went through the frozen rv_continuous machinery
(self.div_clock.logpdf/logsf, self.death_clock.logpdf/logsf) for that -
the same class of overhead already fixed in division_probability(),
just not applied here. Replaced with direct vectorized formulas against
scipy.special (gammaln/gammaincc), matching the old output to float
precision (verified numerically) while cutting test_works_through_crossval
from ~14.7s to ~6.3s in isolation.

The div_clock/death_clock properties are left in place since
compare_emissions.py still uses them outside this hot path.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@aarmey
aarmey merged commit 8f08909 into master Sep 12, 2026
2 checks passed
@aarmey
aarmey deleted the worktree-memoized-questing-deer branch September 12, 2026 15:26
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