Uh oh!
There was an error while loading. Please reload this page.
Fix/dtype narrowing - #29
Merged
Merged
Conversation
`indices` is the only nnz-sized array in the VCS layout, and nothing ever chose its dtype: `_construct.compress` copied whatever dtype the input scipy array carried, and `write_ivcs_elem` recorded the same. scipy hands out int64 indices for any array with enough nonzeros, so a 33k-gene minor axis was routinely stored -- in memory, on disk, and in every kernel that walks it -- at 8 bytes per nonzero instead of 4. `_rapid_load._filter_and_compact` had the same conflation in a sharper form: one `idx_dtype`, keyed off nnz, applied to both `new_indptr` (indexed by nonzero count, genuinely needs int64 at scale) and `out_indices` (gene indices, bounded by the gene count). Crossing INT32_MAX nonzeros silently doubled the largest allocation in the function for no reason. Adds `_indexutils.smallest_index_dtype(n)` as the single place that rule lives, and applies it at each point an index array is sized: - `_construct.compress` takes `n_minor` and narrows up front, so the wide buffer is never allocated rather than allocated and then shrunk. - `_VCSBase.__init__` narrows as the construction choke point -- after the bounds check, so an out-of-range index is still rejected rather than truncated, and a no-op (no copy) when the dtype is already right. - `write_ivcs_elem` re-derives the dtype a reader will rebuild `indices` as, instead of trusting the array it was handed. - `_filter_and_compact` keys its pointer and column-index dtypes off nnz and the kept-gene count separately. - `transpose_major` now uses the shared helper for the rule it already applied inline. Narrowing never widens: indices already stored in something smaller than int32 are left alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#22/#23 added astype, native minor-axis selection, and elementwise add/sub/multiply (which rebuild through scipy). Every one of them constructs its result with `type(self)(...)`, so all of them inherit the narrowing in `_VCSBase.__init__` for free -- no per-method change was needed, which is the payoff for putting the rule at the construction choke point rather than in `from_scipy`. Verified rather than assumed: an int64-indexed input stays int32 through astype, `v[:, cols]`, both-axes selection, scalar mul/div/neg, add/sub against another VCS array, copy, log1p, `_transpose_major` and `T`. The scipy round-trip in the elementwise path is the one that could plausibly have handed back int64, so it gets its own case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fishidaho
marked this pull request as draft
September 3, 2026 21:14
Comments now state what the code does rather than the reasoning behind it. Index-dtype tests drop the cases the code trivially guarantees, keeping the boundary, the truncation risks, the roundtrips, and the paths that build a new array.
fishidaho
commented
Sep 4, 2026
ContributorAuthor
All of the active PRs right now are essentially load-bearing for all of the issues I have open right now. I think merging these in prior to an initial v0.1.0 PyPi release would be ideal. None of them actually change the existing API; however, the future issues will likely touch the API or introduce new API function. |
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.
Size index arrays by the axis they address, not the array beside them
What's wrong
indicesis the onlynnz-sized array in the VCS layout, and nothing ever picked its dtype. Instead, every path just inherited whatever the input carried:_construct.compress(src/vsparse/_construct.py) copiedminor_indices.dtypestraight into its output buffer.write_ivcs_elem(src/vsparse/_io.py) recordednp.dtype(v.indices.dtype).name, so a wide in-memory array stayed wide on every future read.scipy hands out int64 indices for any array with enough nonzeros, so a 33k-gene minor axis was routinely stored at 8 bytes per nonzero instead of 4. This was found to be the case in memory, on disk, and in every kernel that walks it.
_rapid_load._filter_and_compacthad the same conflation in a sharper form. Oneidx_dtype, keyed off nnz, was applied to two arrays with completely different bounds:What this changes
Adds
_indexutils.smallest_index_dtype(n)as the single place the rule lives and applies it at each point an index array gets sized:_construct.compress(newn_minorarg)_VCSBase.__init__write_ivcs_elem_filter_and_compact→new_indptr_filter_and_compact→out_indicestranspose_majorVerification
tests/test_index_dtypes.py(new, 58 cases across the shared shape fixtures):INT32_MAXand one past it;from_scipyon an int64-indexed input produces int32indices, andindices.nbytes == 4 * nnz;INT32_MAXstill gets int64 (tiny nnz against a huge shape, so the test stays cheap);vcsc/ivcscwrite→read round trips preserve values and shape while coming back narrow;_filter_and_compactpicks int32 for both when both fit, and keeps gene indices at int32 when the nnz-keyed half is forced to int64 — the regression case, since allocating a >INT32_MAX-nonzero matrix isn't testable directly.Full suite: 819 passed, 37 skipped.
ruff check src testsclean;ty checkintroduces no new diagnostics (the one pre-existing
no-matching-overloadon the read path is untouched).
Before/after on the headline path: