perf(rag): O(k) integer-array fancy-index gather in _gather_indices (closes #69) - #70
Merged
Merged
Conversation
Design doc for eliminating the O(n) np.arange(n)[where] allocation in Ragged._gather_indices, with a benchmark-first harness (correctness oracle + n-sweep + pyinstrument) per the performant-py-rust workflow. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Contributor
Merging this PR will regress 1 benchmark
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_bench_baseline_ragged_cres |
6.3 ms | 7 ms | -10.38% |
| ⚡ | test_bench_baseline_ragged_short_alleles |
3.7 ms | 1.9 ms | +95.34% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing worktree-issue-69-ragged-gather-perf (5185ba9) with main (decb8f5)
Pre-existing lint (newer clippy) flagged the ACGT byte array in parallel_lut_matches_serial; use a byte-string literal. Unblocks the prek CI check. No behavior change. Verified: cargo fmt --all --check and cargo clippy --all-targets -- -D warnings both clean in the dev env (the exact hook commands); local pre-commit cargo hooks skipped due to a stale PYO3_PYTHON in the bare shell, not a code issue.
d-laub
marked this pull request as ready for review
July 22, 2026 05:02
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
Eliminates the
O(n)np.arange(n)[where]allocation inRagged._gather_indices, so an integer-array fancy-index gather ofkrows from ann-row ragged buffer now runs in O(k), independent ofn. Closes #69.The change is a behavior-preserving performance refactor confined to the integer-array
elsebranch of_gather_indices. The slice fast-path, bool-mask branch, Rust_ragged_selectkernel, and numpyImportErrorfallback are untouched. No public signature change, no maturin rebuild, noSKILL.mdchange.Approach
The old branch resolved indices via
np.arange(n)[where], which allocated a full-length array over allnrows just to selectkof them (and implicitly did negative-normalization + bounds-checking as a side effect). The new branch does direct O(k) vectorized NumPy over only thekselected indices:np.asarray(where)→ integer-dtype guard → normalize negatives → explicit bounds-check raisingIndexError(the numpy indexing contract).Results
benchmarks/bench_ragged_gather.py(transitional harness + correctness oracle):rag[idx]: now flat in n (53 µs @ n=4K vs 57 µs @ n=4.2M), where it previously rose with n. This flat curve is the red→green signal.reference_resolve(old arange logic)== candidate_resolve(new O(k) logic) on all representative + edge cases; both raiseIndexErroron OOB±/float.candidate_resolve's body is the verbatim source of the shipped branch.Tests
tests/test_ragged_core.py— permanent regression coverage for the integer-array gather path (previously untested for OOB / float): int-array selection, negative-normalization parity, OOB positive/negative, empty int64 array, empty Python list, list-of-one parity, float→IndexError, bool-list rejection. 84 passed.rag[[True, False, True]](a plain Python list of bools) now raisesIndexErrorinstead of being treated as a boolean mask.Before this branch, a bool-list incidentally worked as a mask (a side effect of delegating to
np.arange(n)[where]). It now falls into the integer-index path and is rejected. I kept the rejection deliberately, because_where_is_boolalready defines seqpro's mask contract as "a mask is annp.ndarrayof bools" — bool-lists working was never designed, and rejecting them (a) is consistent with that contract, (b) fails loudly rather than silently, and (c) keeps this perf fix confined to the integer branch instead of pulling in numpy's bool-indexing quirks. Pinned bytest_getitem_bool_list_raises. The supported ndarray-mask path (rag[np.array([...])]) is unaffected.If you'd rather restore full numpy parity (route bool-lists through the mask path), say so and I'll adjust — it's a small follow-up.
Empty-list indexing (
rag[[]]→ empty selection;rag[np.array([])]float ndarray →IndexError) is preserved exactly.🤖 Generated with Claude Code