Splice sparse sources into views of sparse arrays in copyto! - #799
Open
ViralBShah wants to merge 3 commits into
Open
ViralBShah wants to merge 3 commits into
ViralBShah wants to merge 3 commits into
Conversation
Fixes #401. `copyto!` into a column view of a sparse matrix, or into a view of a sparse vector, fell through to the generic element-wise fallback, which assigns every element through `setindex!` at O(nnz) per insertion. The stored entries of the parent within the covered index range are now replaced in one splice by those of the source, shifting the column pointers for a matrix, so the cost is proportional to the stored entries moved. A dense source is compressed first; a fixed parent keeps the generic path. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V6EdE4F3CCGE3vxr8gQYKf
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #799 +/- ##
==========================================
+ Coverage 92.53% 92.58% +0.04%
==========================================
Files 12 12
Lines 8404 8546 +142
==========================================
+ Hits 7777 7912 +135
- Misses 627 634 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Two trials per shape instead of five, one fewer shape and source per loop, and drop a vacuous assertion. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V6EdE4F3CCGE3vxr8gQYKf
They were already nested inside `@testset "copy[!]"` but wrapped in a testset of their own; every other member of that testset is a bare `let` block. Dissolve the wrapper into two `let` blocks in the same idiom, one for matrix column views and one for sparse vector views. Drops the issue's literal repro, `copyto!(@view(M[:, 2]), spzeros(3))` on an all-zero matrix. #401 is about that call taking the slow fallback, not about the result being wrong, so the assertion covered nothing the random loop does not already cover with empty sources and empty columns; the `@allocated` bound is what actually guards the fix. 157 assertions before, 156 after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADyZUkMa8jqP4seSkQQZ9o
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
1 participant
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 #401.
copyto!into a column view of a sparse matrix (copyto!(@view(M[:, 2]), v)) or into a view of a sparse vector fell through to the generic element-wise fallback, which assigns every element throughsetindex!at O(nnz) per insertion. The stored entries of the parent within the covered index range are now replaced in one splice by those of the source, shifting the column pointers for a matrix, so the cost is proportional to the stored entries moved: copying a one-entry vector into a column of a 10^6-row matrix allocates 224 bytes.Sources may be sparse vectors, views of sparse arrays (including another column of the same matrix), or dense vectors, which are compressed first. A source shorter than the view leaves the rest of the view untouched, as for dense
copyto!, and a longer one throws aBoundsError. The source's entries are always copied before splicing, so a source that shares storage with the parent, or whose index vector would otherwise be shifted in place, is left intact. A fixed parent keeps the generic path.Tests compare against dense
copyto!over random shapes, lengths and index types for column views, partial and whole vector views, and check the stored pattern stays sorted and unique, stored zeros are kept, the source is unmodified, the issue's example, and the allocation bound.🤖 Generated with Claude Code
https://claude.ai/code/session_01V6EdE4F3CCGE3vxr8gQYKf