Skip to content

Select minor duplicate indices - #35

Merged
aarmey merged 4 commits into
mainfrom
fix/select-minor-duplicate-indices
Sep 4, 2026
Merged

Select minor duplicate indices#35
aarmey merged 4 commits into
mainfrom
fix/select-minor-duplicate-indices

Conversation

@fishidaho

Copy link
Copy Markdown
Contributor

Fix silent data loss when a minor-axis selection repeats an index

What's wrong

arr[:, [1, 1, 3]] returns zeros where column 1's second copy should be. Just wrong values, in a shape that looks right.

v[:, [1, 1, 3]] want got
[[ 2 2 4] [[ 0 2 4]
[ 6 6 8] [ 0 6 8]
[10 10 12]] [ 0 10 12]]

This is a regression introduced by #23. Before it, the same expression fell through to to_scipy()[...], and scipy fans duplicate fancy indices out correctly. Verified directly against 45ce15d, where it returns the right answer.

Duplicate indices aren't exotic here: any resampling-with-replacement, any bootstrap over cells or genes, any join that maps several output columns onto one source column produces them. The failure is silent, so it surfaces as a downstream result being subtly wrong rather than as a crash.

Why it isn't a small patch

The cause is structural. _select_minor builds one old → new lookup array:

remap=np.full(self.n_minor, -1, dtype=np.int64)
remap[idx] =np.arange(n_minor_new, dtype=np.int64)

A repeated index writes that slot twice and only its last destination survives. A single array can't express one-to-many, so no amount of care around this formulation fixes it.

The fix inverts the selection instead. fanout/offsets/positions give, for each original minor index, every output position it maps to; each stored element then emits one entry per destination.

It also removes an nnz-sized temporary

Deriving each major slice's slot count with np.searchsorted on major_ptr drops the np.repeat(np.arange(n_unique), ...) the old version needed, which was nnz-sized. Peak allocation for a 2e6-nonzero selection:

before 62.27 MB
after 4.78 MB (13x)

fishidahoand others added 4 commits September 3, 2026 11:25
`arr[:, [1, 1, 3]]` returns zeros where column 1's second copy should be.
No error, no warning -- just wrong values, in a shape that looks right.
v[:, [1, 1, 3]] want got
[[ 2 2 4] [[ 0 2 4]
[ 6 6 8] [ 0 6 8]
[10 10 12]] [ 0 10 12]]
This is a regression. Before #23 the same expression fell through to
`to_scipy()[...]`, and scipy fans duplicate fancy indices out correctly;
verified against 45ce15d, where it returns the right answer.
The cause is structural rather than an off-by-one: `_select_minor` builds a
single old -> new lookup array,
remap = np.full(self.n_minor, -1); remap[idx] = np.arange(len(idx))
and a repeated index writes that slot twice, so only its last destination
survives. One array cannot express one-to-many, so the fix is to invert the
selection instead: `fanout`/`offsets`/`positions` give, for each original
minor index, every output position it maps to, and each stored element emits
one entry per destination. Two passes (count, then fill) via
`_ops.minor_select_counts`/`minor_select_fill`, so each surviving slot writes
a disjoint range and the passes parallelize.
Deriving each major slice's slot count with `np.searchsorted` on `major_ptr`
also drops the `np.repeat(np.arange(n_unique), ...)` this used to build,
which was `nnz`-sized. Peak allocation for a 2e6-nonzero selection falls
from 62.27 MB to 4.78 MB, so this closes ISSUE-30 as well -- not a separate
concern, just what the correct implementation happens not to need.
All 1066 tests from main pass unchanged. New coverage coming from the
fan-out semantics: five duplicate patterns, duplicates on either axis and
both at once, 25 random selections with repeats compared against scipy
directly, plus the selections that already worked (sorted, reversed, empty,
negative, slices, boolean masks) so the fix can't silently narrow them.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The fan-out explanation moves from a block comment into short notes inside
the kernels. Duplicate-pattern sweep drops to three cases and the type/dtype
assertions go, since construction guarantees them.
@fishidahofishidaho self-assigned this Sep 4, 2026
@fishidaho
fishidaho marked this pull request as ready for review September 4, 2026 00:28
@aarmey
aarmey merged commit 03222e4 into mainSep 4, 2026
5 checks passed
@aarmey
aarmey deleted the fix/select-minor-duplicate-indices branch September 4, 2026 01:52
Sign up for freeto 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.

2 participants

@fishidaho@aarmey