Repository navigation
fix: downsample_counts(total_counts=...) silently did nothing for Fortran-ordered dense X - #4408
Open
entity-0-0-2 wants to merge 2 commits into
Open
entity-0-0-2 wants to merge 2 commits into
entity-0-0-2 wants to merge 2 commits into
Conversation
…ntiguous x.reshape(-1) can only return a view for C-contiguous input. For Fortran-ordered dense X it silently returns a copy, so the in-place downsampling of the flattened view never reached adata.X and the call returned unchanged data. Detect the discarded copy and write the downsampled flat result back into the original layout, which also makes F-order results identical to C-order results for the same rng.
entity-0-0-2
force-pushed
the
fix/downsample-fortran-order
branch
from
October 9, 2026 05:20
f1cc275 to
11b272f
Compare
This branch has not been deployed
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.
downsample_counts(total_counts=...)silently does nothing for Fortran-ordered denseX#4406test_downsample_total_counts_fortran_order, red onmain, green with the fixdocs/release-notes/4408.fix.mdis includedPain
sc.pp.downsample_counts(adata, total_counts=...)with a denseXthat is Fortran-ordered returns silently and leaves the data completely unchanged. No error, no warning: a pipeline that asks for 1000 total counts keeps all 7505 and every downstream result is quietly wrong. From the linked issue:Root cause
_downsample_total_countsinsrc/scanpy/preprocessing/_simple.pyflattens dense input withv = x.data if isinstance(x, CSBase) else x.reshape(-1)(line 949 onmain), then downsamplesvin place and returnsspc.xon the assumption thatvaliasesx(the invariant stated in the adjacent comment). That assumption only holds for C-contiguous input:reshape(-1)must iterate in C order, so for an F-contiguous array numpy cannot return a view and silently returns a copy. The numba kernel then zeroes and refills the copy, and the untouched original is returned. The same discarded-copy pattern has been there since the dense path was introduced in commit 33b7090 (PR #474) asX.view().reshape(...), and was carried through the refactor in 9de11b1 (PR #3983). Thecounts_per_cellpath is not affected becauserows[rowidx]is a basic slice that writes through in any memory order, which is also why the C-order test suite never caught it.The fix
Two lines in
_downsample_total_counts: after the in-place downsample, ifvdoes not share memory withx(dense only, sparse.dataalways aliases), write the downsampled flat result back into the original layout:This keeps C-order behavior byte-for-byte identical (no extra work when
vis a view), preserves the input memory order, and makes F-order results exactly equal to C-order results for the same rng seed.Test
test_downsample_total_counts_fortran_orderintests/test_preprocessing.py: runs the same downsample onnp.ascontiguousarrayandnp.asfortranarraycopies, asserts the F-order sum is actually reduced to the target, asserts elementwise equality with the C-order result, and asserts the input stays F-contiguous.Before (on
main):After:
The new test fails on
main(assert np.float32(7505.0) == 1000) and passes with the fix;pytest tests/test_preprocessing.pyis otherwise green.How this was found
This fix was located with Vestiges Causal Walk (https://github.com/samvallad33/vestige): the failure was recorded as a symptom memory, a backward causal walk over recorded edges ranked the mechanism (the order-dependent
reshapeview/copy switch), the unverified aliasing contract, and the origin commit, and the ranked causes were verified against a local repro before writing any code.Fixes #4406