Skip to content

perf: don't apply fancy indexing if split can be a slice.#235

Open
selmanozleyen wants to merge 4 commits into
scverse:mainfrom
selmanozleyen:perf/dont-apply-fancy-indexing-if-not-needed
Open

perf: don't apply fancy indexing if split can be a slice.#235
selmanozleyen wants to merge 4 commits into
scverse:mainfrom
selmanozleyen:perf/dont-apply-fancy-indexing-if-not-needed

Conversation

@selmanozleyen

Copy link
Copy Markdown
Member

In my use case when batch_size=1, splits becomes something like this = [[0],[1],[2],...]. For each split I think this becomes a fancy indexing operation and can be costly. In my case every row is 40mb. And I want to have some preloaded rows but turns out each in_memory_data[split] is costly even though it fetches for one row.

This is a bit more of a generalized case. But I added (split[-1] - split[0] == len(split) - 1 check so it returns early but if you think we should avoid this check we can also just have a special case for batch_size=1

@selmanozleyen selmanozleyen changed the title perf: Don't apply fancy indexing if split can be a slice. perf: don't apply fancy indexing if split can be a slice. Jun 15, 2026
@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.92%. Comparing base (796e789) to head (d520726).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
- Coverage   93.48%   91.92%   -1.57%     
==========================================
  Files          15       15              
  Lines        1397     1399       +2     
==========================================
- Hits         1306     1286      -20     
- Misses         91      113      +22     
Files with missing lines Coverage Δ
src/annbatch/loader.py 87.79% <100.00%> (-3.01%) ⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@selmanozleyen selmanozleyen added the skip-gpu-ci Whether gpu ci should be skipped label Jun 15, 2026
@selmanozleyen selmanozleyen requested a review from ilan-gold June 15, 2026 14:44
@ilan-gold

ilan-gold commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

In my use case when batch_size=1

Just to be clear, then

sel = slice(sel[0], sel[-1] + 1)

becomes a length-1 slice? And this is faster than fancy-indexing? Feels like a bug almost (not with us)

@selmanozleyen

Copy link
Copy Markdown
Member Author

In my use case when batch_size=1

Just to be clear, then

sel = slice(sel[0], sel[-1] + 1)

becomes a length-1 slice? And this is faster than fancy-indexing? Feels like a bug almost (not with us)

Yes its setup: preloadnchunks=x,batchsize=1,chunksize1.
It is because I have irregular batch sizes but I also want the preloadnchunks of annbatch. Like unique donors corresponding to a batch can be arbitrary. I asked Lukas if its fine if we sample fixed sized donors and sample the corresponding cells from the donors so we have fixed sized batches and it would be somewhat mathematically equivalent. But I wanted to implement whatever lucas already has 1-1

@selmanozleyen

Copy link
Copy Markdown
Member Author

I checked and for numpy it creates a copy in case of fany indexing of the size of the index. Vs compared to slices which only returns a view. Since each element is 40mb even allocating one row can be costly. In short if its a slice its a view if its a list its a copy view vs copy. Makes sense that they don't have special cases because you would want consistent behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-gpu-ci Whether gpu ci should be skipped

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants