feat: sparse storage add/sub - #442
Open
henryiii wants to merge 2 commits into
Open
Conversation
Collaborator
|
I am leaning towards calling this a feat instead of a fix, you are special-casing on node_access. I merged the other one, that was more straight forward. Not sure whether the release manager will accept so many fixes in the beta phase. This is supposed to be the time for small fixes. |
Collaborator
|
Please rebase. |
operator+= and operator-= on a histogram with map-based storage iterated the full logical index range, so adding two sparse histograms touched every cell. Iterate only the stored nodes when the rhs storage is map-based, via the new has_node_access trait. operator/= keeps the full loop on purpose: dense semantics require NaN cells where the divisor is zero. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
Skip keys at or beyond the storage size in the sparse add/sub paths; a corrupt serialization archive could otherwise cause an out-of-bounds write on a dense left-hand side. Also correct the allocator baselines in the new storage_adaptor tests to use the net counter. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
henryiii
force-pushed
the
fix/sparse-storage-arithmetic
branch
from
July 30, 2026 13:48
16051fc to
6d95349
Compare
Contributor
Author
|
Since the other one didn't protect on |
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
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.
This fixes #440. Closes #441 (could be rebased after that one, if you prefer). This keeps
+=/-=from spiking memory on large sparse histograms.I wasn't sure about protecting for types that don't have
==. Everything except/=and==is already protected, similar to the current code here, while/=/==don't have that protection (like #441), so they can't be used on types without==(but everything else works, including+=). The concept claims it requires==. I stuck with protecting it, but can remove that if you'd prefer. I could also make/=work for==types (always insert) as a separate PR if you want parity.Assisted-by: ClaudeCode:claude-fable-5