Uh oh!
There was an error while loading. Please reload this page.
Minor sums memory - #30
Merged
Merged
Conversation
`_VCSBase._minor_sums` expanded the value-compressed layout back to one float64 per nonzero before reducing it: expanded = np.repeat(self.values.astype(np.float64), group_sizes) return np.bincount(self.indices, weights=expanded, minlength=self.n_minor) That `np.repeat` is scratch space for a reduction that never needs to keep it -- and it undoes, transiently, the exact compression the layout exists to provide. Any `sum(axis=...)` over the minor axis pays it, including the one `NormalizedViewBase.__init__` makes on every view construction. Replaces it with a parallel numba scatter (`_ops.minor_sums`) that walks the value groups directly. Group index ranges are disjoint, so threads take contiguous blocks of groups and accumulate into thread-local rows, summed at the end -- no write hazard, and nothing nnz-sized is ever allocated. The thread-local accumulators are the obvious way to reintroduce the same problem in a new shape (`nthreads * n_minor * 8` bytes), so the thread count is capped to keep that block under a fixed 64 MiB budget: a minor axis wide enough that even two accumulators would blow it runs serially instead. Measured on 1e6 nonzeros, peak allocation drops from 16.00 to 0.20 bytes per nonzero; on 4e7 nonzeros the reduction also runs 11x faster (0.139s -> 0.012s), since it moves far less memory and now uses every core. Also adds the direct `sum()` coverage the axis reductions never had -- both axes against a dense reference, the all-zero case, and a regression test asserting the call path allocates nothing nnz-sized. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#22 added per-axis max/min and getnnz. Both take the same shape as the `_minor_sums` this branch was already fixing, and both were written the same way -- so the defect this PR exists to remove arrived in three new places at once: _minor_reduce np.repeat(values, group_sizes) then ufunc.at 16.07 B/nnz _minor_nnz np.bincount(int32 indices) promotes to intp 8.00 B/nnz Leaving those while fixing `sum` would make the PR incoherent, so they get the same treatment. `_ops.minor_extrema` computes the extremum over stored values and the per-index count in one parallel pass over the value groups; `_ops.minor_counts` does the count alone. Measured on 2e6 nonzeros, against the implementations they replace: max(axis=0) 32.01 MB -> 0.39 MB (81x) 15.3x faster min(axis=0) 32.01 MB -> 0.39 MB (81x) getnnz(axis=0) 16.00 MB -> 0.20 MB (81x) 8.7x faster Output is identical in every case, and matches a dense reference. #22's own tests for these methods pass unchanged -- the implicit-zero correction stays in `_minor_reduce` rather than moving into the kernel, so the semantics those tests pin are untouched. The accumulator budget now takes a per-element size, since the extrema kernel keeps an extremum *and* a count per slot: 64 MiB still bounds the block, it just buys fewer threads for a wider accumulator. Renames tests/test_reductions.py to test_reduction_memory.py -- #22 added test_reductions_and_arith.py for the semantics, and two files a letter apart covering different concerns is a trap. The docstring now says which is which. Not fixed here: `_select_minor` (new in #23) has the same nnz-sized temporary, at 15.07 B/nonzero. It's an indexing path rather than a reduction, so it's filed separately as ISSUE-30 instead of widening this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fishidaho
marked this pull request as draft
September 3, 2026 21:14
Merged
`_weighted_bincount` and `_gene_detection_counts` size their thread-local block by the full thread count, so it grows with both the feature axis and the machine: 96 MB at 250k features on 48 threads. They now share the same cap as the other scatter kernels. `accumulator_threads` loses its underscore, since it is no longer local to one module. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
minor_sums and minor_counts were one-line wrappers that only supplied the thread count, so the kernels now take the public name and callers pass it. minor_extrema keeps a wrapper because it reduces the per-thread partials. Tests drop the internal-call and sweep cases, keeping the dense comparisons, the implicit-zero and integer-sentinel edges, the float64 accumulator, the budget, and the memory bound.
aarmey
approved these changes
Sep 4, 2026
# Conflicts: # src/vsparse/_ops.py # src/vsparse/_rapid_load.py
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
2 participants
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.
Reduce over the minor axis without an nnz-sized temporary
What's wrong
_VCSBase._minor_sums(src/vsparse/_base.py) expanded the value-compressed layout back to one float64 per nonzero before reducing it:That
np.repeatis an nnz-sized float64 array allocated purely as scratch for a reduction that never needs to keep it. It transiently undoes the exact compression the layout exists to provide. Everysum(axis=...)over the minor axis pays it, including the oneNormalizedViewBase.__init__makes on every view construction, so it's on the path of ordinary normalization too.What this changes
_ops.minor_sums— a parallel numba scatter that walks the value groups directly. Value-group index ranges are disjoint, so threads take contiguous blocks of groups and accumulate into thread-local rows that are summed at the end: no cross-thread write hazard, and nothing nnz-sized is ever allocated.Verification
Measured directly, 1e6 nonzeros, same output both ways:
At 4e7 nonzeros it's also 11× faster (0.139s → 0.012s) — less memory moved, and it now uses every core instead of one.
Review notes
values/value_ptr/indicesbut notmajor_ptr. Minor sums genuinely don't care which major slice a group belongs to, which is also true of the implementation being replaced.