Speed up the test suite without dropping coverage - #1011
Merged
Merged
Conversation
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>
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.
Summary
stateCommon.pygrabbed the wrong Cython fused-function overload for the digamma function (psi) —__pyx_fuse_0psiispsi's complex128 specialization, not the real-valued one (__pyx_fuse_1psi). Calling the complex overload through a real-valuedctypes.CFUNCTYPEreads garbage for the argument's missing imaginary half. This silently biasedgamma_mle_closed_form's shape-parameter MLE (its "exact, instantaneous" uncensored fast path — e.g. a syntheticGamma(4, 2)sample recovered shape 3.84 instead of 3.98) and threw offgamma_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 slowertrust-constrsolver, which was the dominant remaining cost intest_cvandtest_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.StateDistribution.division_probability()'s integrand to usescipy.special(gammaln/gammaincc) directly instead of scipy.stats frozen-distributionpdf/sf, which was ~100x more expensive per call inside thequad()integration used every M-step.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 frozenrv_continuousmachinery. Verified the new output matches the old to float precision. Cutstest_works_through_crossvalfrom ~14.7s to ~6.3s in isolation on top of thedivision_probabilityfix.pytest-xdist(-n auto, already a dev dependency but unused) viaaddoptsinpyproject.toml, and pinnedOMP_NUM_THREADS/OPENBLAS_NUM_THREADS/MKL_NUM_THREADS/NUMEXPR_NUM_THREADSto 1 in a newlineage/tests/conftest.pyso xdist workers don't fight scipy's own internal BLAS threading for cores.Remaining known costs (not addressed here)
gamma_estimator's njit functions pays a one-time ~3-4s cost per process the first time each type signature is hit; under-n autoseveral workers re-pay it independently. Would needcache=Trueon the njit decorators to amortize across processes/runs.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 oftest_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 pointuv run ruff checkon touched files — passespsifix numerically: compared the JIT function against its.py_funcpure-Python twin on identical inputs (they now agree; they didn't before), and checked__pyx_fuse_1psiagainstscipy.special.psidirectly across several valueslogpdf()'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 rowsdivision_probability()still matches the existingtest_sub_densities_sum_to_one/test_estimator_recovers_parameters/test_phase_estimator_recovers_parameterscorrectness checks🤖 Generated with Claude Code