Implement sparse + sparse matrix addition for CSC, CSR, and COO formats #27
Conversation
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
Co-authored-by: albertomercurio <[email protected]>
Co-authored-by: albertomercurio <[email protected]>
Copilot
AI
changed the title
[WIP] Implement sum method for AbstractDeviceSparseMatrix
Implement sparse + sparse matrix addition for CSC, CSR, and COO formats
Nov 16, 2025
Copilot finished work on behalf of
albertomercurio
November 16, 2025 12:28
Owner
albertomercurio
left a comment
albertomercurio
left a comment
There was a problem hiding this comment.
@copilot Overall it seems a good job. I'm wondering if we can also implement the sum among transposed/adjointed matrices, like +(A::DeviceSparseMatrixCSC, B::Transpose{T, <:DeviceSparseMatrixCSC}), and in case how to do it. Should one convert the transposed matrix to CSR? If so, can the sum be efficiently computed between a CSC and CSR?
If not, are there other direct ways to do it, like the methods done for the mul! or dot functions?
Comment on lines
372
to
391
| nnz_A = nnz(A) | ||
| nnz_B = nnz(B) | ||
| nnz_total = nnz_A + nnz_B | ||
|
|
||
| # Allocate result arrays | ||
| rowind_C = similar(getrowind(A), nnz_total) | ||
| colind_C = similar(getcolind(A), nnz_total) | ||
| nzval_C = similar(nonzeros(A), Tv, nnz_total) | ||
|
|
||
| # Copy entries from A | ||
| rowind_C[1:nnz_A] .= getrowind(A) | ||
| colind_C[1:nnz_A] .= getcolind(A) | ||
| nzval_C[1:nnz_A] .= nonzeros(A) | ||
|
|
||
| # Copy entries from B | ||
| rowind_C[(nnz_A+1):end] .= getrowind(B) | ||
| colind_C[(nnz_A+1):end] .= getcolind(B) | ||
| nzval_C[(nnz_A+1):end] .= nonzeros(B) | ||
|
|
||
| return DeviceSparseMatrixCOO(m, n, rowind_C, colind_C, nzval_C) |
Owner
There was a problem hiding this comment.
@copilot Here you are just concatenating the nonzero values, but they can also be repeated and one should merge them instead. Could you take into account also that?
Check that you merge also the other formats.
Contributor
Author
There was a problem hiding this comment.
Fixed in commit 778e354. COO addition now properly merges duplicate entries by:
- Concatenating and sorting by (row, col)
- Marking unique entries
- Compacting with duplicate merging using
kernel_compact_coo!
All three formats (CSC, CSR, COO) now correctly merge duplicates. Verified with tests.
…ndency Co-authored-by: albertomercurio <[email protected]>
Contributor
Author
Good question! Adding support for transposed/adjointed matrices would be valuable. Here are the options: Option 1: Convert CSC transpose to CSR (Most efficient)
Option 2: Use existing pattern from mul!/dot (Also efficient)
Option 3: Convert to same format (Simpler but less efficient)
Recommendation: Option 1 (CSC+CSR merge) or Option 2 (direct transpose handling) would be most efficient. Option 2 follows the existing mul! pattern more closely. Should I implement one of these approaches? Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot finished work on behalf of
albertomercurio
November 16, 2025 15:50
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
==========================================
+ Coverage 82.64% 85.04% +2.40%
==========================================
Files 14 14
Lines 801 1030 +229
==========================================
+ Hits 662 876 +214
- Misses 139 154 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Benchmark Results'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.30.
| Benchmark suite | Current: 778e354 | Previous: 862b4d5 | Ratio |
|---|---|---|---|
Kronecker Product/JLArray/CSC |
16593217 ns |
12002090.5 ns |
1.38 |
Kronecker Product/JLArray/COO |
3484290 ns |
2334051 ns |
1.49 |
Kronecker Product/JLArray/CSR |
16622938 ns |
11984809 ns |
1.39 |
This comment was automatically generated by workflow using github-action-benchmark.
Sign up for free
to 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.
Implementation Complete: Sparse Matrix Addition
Successfully implemented the
+(A::AbstractDeviceSparseMatrix, B::AbstractDeviceSparseMatrix)operation for CSC-CSC, CSR-CSR, and COO-COO formats.Completed Tasks:
kernel_count_nnz_per_col_csc!insrc/matrix_csc/matrix_csc_kernels.jlkernel_merge_csc!insrc/matrix_csc/matrix_csc_kernels.jl+(A::DeviceSparseMatrixCSC, B::DeviceSparseMatrixCSC)insrc/matrix_csc/matrix_csc.jlkernel_count_nnz_per_row_csr!insrc/matrix_csr/matrix_csr_kernels.jlkernel_merge_csr!insrc/matrix_csr/matrix_csr_kernels.jl+(A::DeviceSparseMatrixCSR, B::DeviceSparseMatrixCSR)insrc/matrix_csr/matrix_csr.jlkernel_mark_unique_coo!insrc/matrix_coo/matrix_coo_kernels.jlkernel_compact_coo!insrc/matrix_coo/matrix_coo_kernels.jl+(A::DeviceSparseMatrixCOO, B::DeviceSparseMatrixCOO)insrc/matrix_coo/matrix_coo.jltest/shared/matrix_csc.jltest/shared/matrix_csr.jltest/shared/matrix_coo.jlbenchmark_sparse_sparse_add!inbenchmarks/matrix_benchmarks.jlbenchmarks/runbenchmarks.jlImplementation Details:
CSC/CSR Formats: Two-pass merge algorithm
COO Format: Sort + compact algorithm
GPU Compatibility: Avoided scalar indexing using broadcasting operations (e.g.,
colptr_C[1:1] .= one(Ti))Test Results:
✅ 1478 tests passed (Base Array + JLArray backends)
✅ All sparse + sparse addition tests pass
✅ Works correctly with overlapping and non-overlapping entries
✅ Proper dimension checking and error handling
✅ GPU-compatible (no scalar indexing issues)
✅ COO format now properly merges duplicates
Original prompt
AbstractDeviceSparseMatrix#16💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.