Uh oh!
There was an error while loading. Please reload this page.
Normalized view stats - #31
Merged
Merged
Conversation
A normalized view's statistics are computed once, at construction, over the
whole array it wrapped. `__getitem__` then applies those parent statistics
to whatever sub-block is asked for -- while its docstring described the
result as though the block were self-consistently normalized ("the
transform/centering formula is then applied to that small block directly").
It isn't, and the gap is not small. On a mixed population -- two cell types
differing in read depth and marker expression -- indexing the view for one
cell type differs from normalizing those cells by **52% relative Frobenius
norm**. Read as "the normalized data for these cells", that silently fits
downstream analysis to different data than the caller believes.
Rather than redefine what indexing returns, this keeps the two operations
and names them:
- `__getitem__` stays a window into this view's matrix -- exactly
`toarray()[key]` without materializing the full matrix -- and now says so,
including what it is *not*.
- `select(rows, cols)` is new: it renormalizes the selected sub-array on its
own terms, equivalent to `arr[sel].normalized()`, and returns a view so it
still composes with `@`/`toarray()`.
Recomputation was deliberately not made the default for indexing, because
it is not uniformly more correct: `row_scale` is a per-cell total over the
columns present, so recomputing after a *gene* selection re-derives read
depth from just those genes, which is usually wrong. Making the caller ask
for it keeps that choice explicit; `select`'s docstring spells the caveat
out. Choosing per-selection semantics properly is v0.4's selection-algebra
work.
`select` also fills a real gap: selecting rows of a VCSCArray is a
minor-axis selection, which drops out as scipy with no `.normalized()` at
all, so the raw-array route isn't uniformly available.
Tests pin both contracts against a dense reference, and assert the two
disagree by >10% on a realistic selection -- so the distinction can't be
quietly collapsed by a later change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>#23 changed what a raw array's __getitem__ returns underneath this branch, in two ways that matter here: - A minor-axis selection now stays VCS-native instead of falling out as a scipy array. `select`'s re-wrap is therefore no longer the normal path, just a defensive one -- reworded to match the same defensive re-wrap in `_anndata_class._subset_2d`, and the test comment claiming a VCSCArray row selection "has no .normalized() at all" is simply no longer true. - `arr[rows, cols]` with two index arrays now composes a major- and a minor-axis selection, which is the sub-block semantics `select` wants. It previously fell through to scipy, which broadcasts two index arrays pointwise, and that's why this was written one axis at a time. That workaround is now redundant, so it collapses to a single index operation. Adds a test pinning the outer-vs-pointwise distinction directly, using row and column selections of different lengths -- a pointwise broadcast would fail outright rather than return something subtly wrong, which is what makes this worth keeping a test on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fishidaho
marked this pull request as draft
September 3, 2026 21:13
Docstrings state what the code does. The two both-axes selection tests fold into one that uses different-length keys, which is what actually pins outer semantics.
aarmey
approved these changes
Sep 4, 2026
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Make post-selection normalization explicit and reachable
What's wrong
A normalized view computes
row_scale/gene_scale/col_meanonce, at construction, over the whole array it wrapped(
NormalizedViewBase.__init__,src/vsparse/_norm_common.py).__getitem__then applies those parent statistics to whatever sub-block is asked for. On a mixed population — two cell types differing in both read depth and marker-gene expression — indexing the view for one cell type differs from normalizing those cells by 52% relativeFrobenius norm:
Read as "the normalized data for these cells", this silently fits downstream analysis (parafac2, PCA, anything) to different data than the caller believes.
What this changes
Rather than redefine what indexing returns, this keeps both operations and gives them names:
__getitem__stays a window into this view's matrix — exactlytoarray()[key]without materializing the full matrix — and now says so, including what it is not.select(rows, cols)is new: it renormalizes the selected sub-array on its own terms, equivalent toarr[sel].normalized(). It returns a view, not a dense block, so it still composes with@andtoarray().selectalso fills a real gap rather than just wrapping something that already worked: selecting rows of a VCSCArray is a minor-axis selection, which falls out of_VCSBase.__getitem__as a scipy array with no.normalized()at all. The raw-array route isn't uniformly available;selectis.Verification
tests/test_norm_selection.py(new, 22 cases across both array types):select()matches normalizing the selection directly to < 1e-12 relative error, on the mixed-population fixture;select()== thearr[sel].normalized()route where that route exists;select()returns a composable view —sub @ Bmatches the dense reference;select()with no arguments == the whole view;__getitem__==toarray()[key], pinning the window contract;