Conversation
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.
Fixes #143.
As reported in #143, several source bins can share the same destination index, but the indexed
+=update does not accumulate repeated indices. This drops counts during 16 µm to 128 µm pooling and makes the result depend on the chunk size and input order.This replaces that update with
np.add.at, converting sparse chunks to dense arrays before accumulation. The existing arguments, spatial grid, and float32 output stay unchanged. The change keeps the current chunked implementation; it does not introduce a sparse-only pooling path or remove its dense-memory cost.Verification
The regression test covers 72 combinations: dense, CSR, and CSC matrices; in-memory and backed storage; four chunk sizes, including the default 50,000; and three input orders. It checks exact pooled counts, zero-count bins, gene order, coordinates, and unchanged input counts.
From the repository root, with HEST installed:
That test passes through the actual HEST package on Python 3.11.16/macOS, without mocking its imports.
Real-data validation uses the complete filtered 16 µm matrix from 10x Genomics' human colorectal-cancer Visium HD sample, processed with Space Ranger 3.0.0: 137,051 bins, 18,085 genes, and 266,691,885 UMIs.
The fixed outputs match every bin/gene entry in an independent integer sparse-matrix aggregation, with no lost counts. Changing the chunk size or reversing the barcode order produces the same output matrix. The total also agrees with the original 10x metrics CSV at whole-UMI precision.
The real-data rerun calls the patched function through the installed HEST package. The original comparison uses the unchanged function extracted from upstream commit 3ddb5eaf.
Coordinates are matched by barcode,
microns_per_pixelcomes from the scalefactors file, and pooling usessrc_bin_size_um=16,dst_bin_size_um=128. Counts are not normalized, genes are not selected, and the tissue is not cropped.Dataset files:
binned_outputs/square_016um/filtered_feature_bc_matrix.h5— raw filtered counts.binned_outputs/square_016um/spatial/tissue_positions.parquet— barcode coordinates.binned_outputs/square_016um/spatial/scalefactors_json.json— physical pixel scale.The dataset is provided by 10x Genomics under CC BY 4.0. No dataset files are included in this PR.
Reproduction files
Supporting evidence is kept on a separate fork branch so the upstream diff stays limited to the fix and regression test. These links are pinned to the evidence commit:
Limits
The full WSI reader, image processing, and broader HEST data suite have not been run. This is one real sample, not a revalidation of all 25 slides reported in the issue. The default 50,000-row chunk is covered by the regression test, but was not used for the full real-data matrix because of its dense-memory requirements.