perf(module-score): read the matrix once per gene set, not once per gene - #67
Merged
Conversation
`add_module_score` pulled each gene's row out on its own (`mat[i, :]` in a list comprehension) and handed the stack to `np.mean`. Every assay layer here is CSC, so slicing one row walks the whole column-major matrix — ~22 ms per gene on the THP-1 ECCITE data (18,381 x 20,729, 69.5M nonzeros) — and the default ctrl=100 draws a couple of thousand control genes. 50 of 51 profiled seconds were inside scipy's get_csr_submatrix, called once per gene. cell_cycle_scoring (2 programs) 168.4s -> 0.90s add_module_score (30-gene prog) 51.3s -> 0.51s R's CellCycleScoring takes ~12s on the same data. The spelling is bit-identical to the old arithmetic, deliberately: an indicator-vector matvec is faster still but lands 7.5e-16 away, and summing the CSC selection directly 2.5e-15 away, because each accumulates the columns in a different order. Also fixes a reproducibility defect in the same two lines: the control genes were collected in a `set`, whose iteration order depends on Python's per-process str hashing, so the same object at the same seed summed the control expression in a different order — and scored 9.7e-16 differently — in a different process. A dict now, which is also what R's `unique()` gives. All 20,729 THP-1 Phase calls are unchanged. Co-Authored-By: Claude Opus 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.
The hot spot
add_module_scorepulled each gene's row out on its own and handed the stack tonp.mean:Every assay layer here is CSC, so slicing a single row walks the whole column-major matrix. On the THP-1 ECCITE data (18,381 × 20,729, 69.5M nonzeros) that is ~22 ms per gene, and the default
ctrl=100draws a couple of thousand control genes. The profile is unambiguous:50.1 of 51.1 seconds in one call, invoked once per gene.
The fix
One row selection per gene set, transposed to CSR so the rows are summed in the order they were asked for.
cell_cycle_scoring(THP-1, 2 programs)add_module_score(30-gene program)R's
CellCycleScoringtakes ~12 s on the same data, so this goes from 14× slower than R to an order of magnitude faster.The exact spelling is load-bearing. Measured on the real matrix, all four formulations of the same mean:
ind @ mat(indicator matvec).sum(axis=0).tocsr(),.sum(axis=0)The two faster ones accumulate the columns in a different order. Summing a CSR selection walks the rows in the order they were asked for, which is what the old loop did — so the fast path returns the same bits, not merely the same numbers.
sum(axis=0)/krather than.mean(axis=0)for the same reason: scipy's sparse mean divides on the way through and lands a few ulps away.A reproducibility defect in the same two lines
The control genes were collected in a
set. A mean depends on the order its terms are added, and Python randomisesstrhashing per process, so iterating that set summed the control expression in a different order every run:A
dictnow, which is also R's semantics —AddModuleScoreappliesunique()to the sampled names and indexes the matrix with the result, i.e. first-seen order.THP-1
S.Score,G2M.Scoreand the interferon program move by at most 2.1e-15 against the old code, which had no fixed value to move from, and all 20,729Phasecalls are unchanged.Verification
ruff check shanuz51,ruff check .201,mypy shanuz46 — all unchanged.Mutation testing
set.mean(axis=0)instead ofsum/kThe sort mutation initially survived, and that was the useful finding: the first fixture was Poisson counts, whose partial sums are all exactly representable, so every summation order gives identical bits and an ordering test written on it passes whatever the code does — the same decorative-test failure mode as the SCT test in #59 and the tie-break test in #61. The fixture is now log-normal, where 110 of 200 cells differ under a reordering, and the test asserts that the fixture can tell the two apart before asserting the property.
One further mutation,
mean(axis=0)→sum(axis=0)/nin the dense branch, also survives — verified to be a genuine no-op rather than a gap: numpy'smeanis literallysumthen divide, bit-identical at every size tested.🤖 Generated with Claude Code