Release 0.6.1: sparse outputs at their real size, and split in one pass - #19
Conversation
Every sparse output of subset, split and concat widened `indices` and `indptr` to int64 and dropped the source's compression, because the growable datasets they wrote into forwarded no storage settings. Outputs came out about twice the size anndata writes: 309.7 MB against 146.6 MB for subset in the 0.6.0 benchmark. They now keep the source's dtypes and layout, through the helpers convert already used, moved into subset.py. concat widens to int64 only when the combined matrix needs it. split ran one subset per group. With interleaved groups each subset read the span of its own rows, which was nearly all of X, so X was read k times. The matrix writers now take a list of outputs and share each source block among them; split_store makes one split_h5ad call, batched by the file-descriptor limit. Rows are gathered in one vectorised step rather than a Python loop. Dataframe columns were read through an h5py fancy index, which cost 0.38 s per column per group on 50,000 cells. They are now read as contiguous blocks and selected in memory. ci tier, locally: split 56.7 s -> 1.6 s (anndata 2.3 s), outputs within 0.3% of anndata's size for split, subset and concat. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A patch: two fixes to existing commands and no new surface. The one behavioural change is that a row selection passed to the subset writers must be sorted and unique; every command already produced one, and h5py already required it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0b59e740c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| limit = np.iinfo(np.int32).max | ||
| indices = np.result_type(*[s["indices"].dtype for s in sources]) | ||
| indptr = np.result_type(*[s["indptr"].dtype for s in sources]) | ||
| nnz = sum(int(s["data"].shape[0]) for s in sources) | ||
| if n_minor > limit: | ||
| indices = np.result_type(indices, np.int64) | ||
| if nnz > limit: | ||
| indptr = np.result_type(indptr, np.int64) |
There was a problem hiding this comment.
When valid sparse inputs use an index dtype narrower than int32, the overflow checks still compare only against int32's limit. For example, concatenating two matrices whose indptr arrays are int16 and contain 20,000 nonzeros each leaves pointer_dtype as int16; the final 40,000 offset is then silently wrapped by _write_indptr. Remapped coordinates can overflow similarly when the union exceeds the source indices dtype. Since write_sparse preserves arbitrary supplied integer dtypes, compare the combined bounds with np.iinfo(indices/indptr).max and widen as needed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9124367. Both dtypes are now checked against their own iinfo(...).max and widened to the narrowest of int32/int64 that holds the bound, so the int16 case you describe gives an int32 indptr. It's covered by a unit test and by an end-to-end concat of two int16 inputs, and that test fails on the previous commit.
`_concat_index_dtypes` only compared the combined nnz and the minor dimension with int32's maximum, so inputs with a narrower index dtype kept it past its range: two int16 `indptr`s of 20,000 nonzeros each gave an int16 output whose 40,000 offset wrapped silently. Each is now checked against the chosen dtype's own limit and widened to the narrowest of int32/int64 that holds it. Found by Codex review on #19. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Fixes two defects that predate 0.6.0, both visible in the 0.6.0 benchmark, and bumps the version to 0.6.1.
Sparse outputs were about twice the size they should be
Every sparse output of
subset,splitandconcathad two problems:indicesandindptrwere widened to int64, whatever the source used.dataandindiceswere written uncompressed.convertalready did this correctly. The fix moves its helpers (_sparse_dataset,_growable_like) intosubset.py, and all three commands now use them.subsetandsplitkeep the source's dtypes. That is always safe, because a subset has no more nonzeros and no larger coordinates than its source.concatuses the widest dtype among its inputs. It widens to int64 only when the combined nnz or the minor dimension needs it (_concat_index_dtypes).splitread the whole matrix once per groupsplitran onesubsetfor each group. When groups are interleaved through the file, as samples usually are, each subset read nearly all of X.split_storemakes a singlesplit_h5adcall. Open outputs are capped at a quarter of the file-descriptor limit, with one pass per batch beyond that.subsetgets this too, and it now skips blocks that hold none of the selected rows.Results
ci tier (50k × 20k, lzf), 3 repeats. All "now" figures are from one laptop; the 0.6.0 figures are from the CI runner.
concat_on_disk)The tag will rerun this on the runner and publish it to
docs/BENCHMARKS.md.Behaviour change
Row selections passed to the subset writers must now be sorted and unique, and anything else raises
ValueError. Every command already produces sorted selections, and h5py required the same for fancy indexing.Tests
tests/test_subset_fan_out.py:subsetoutput, and to anndata slicing, for CSR, CSC and dense, h5ad and zarr, obs and vartest_performance.py: split's reads of X must not grow with the number of groups. On the old per-group split it fails with 32× growth.-m "not integration").After merge
Tagging
0.6.1onmainfirespublish.yml(PyPI),quay-on-tag.yml(image) andbenchmark.yml.🤖 Generated with Claude Code