Conversation
You're right, in fact that's how I did it initially in #152 (check out the first commit, which is itself a sub-PR). I removed it afterwards because it felt simpler, but looking at the code again we probably never need to actually materialize |
9dd5400 to
488b3ec
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #156 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 13 13
Lines 1500 1541 +41
=========================================
+ Hits 1500 1541 +41 ☔ View full report in Codecov by Sentry. |
7438bd4 to
bad5931
Compare
|
@gdalle We should check with some benchmarks that we still target the optimized star / acyclic decompression for |
f995396 to
3d50b16
Compare
Benchmark Results
|
gdalle
left a comment
There was a problem hiding this comment.
Can we reduce code duplication by using the same code for transpose and bidirectional_pattern? For instance defining a subroutine like
transpose_pattern!(A, colptr, rowval)which could then also be applied on colptr = view(...) and rowval = view(...) in the case of the bigger matrix (and followed by a vectorized addition to account for the shift)
671c920 to
7e4f62d
Compare
Yes, we could reduce code duplication by implementing a subroutine like That said, introducing the offset logic for rows, columns, and nnz might slightly impact readability. I suggest we merge the PR as it is. |
gdalle
left a comment
There was a problem hiding this comment.
i don't fully understand the code but I trust the tests I added
close #161
If we want to store exactly
[0 A'; A 0], we can't usesymmetric_patternto buildAᵀ.We will have the wrong nonzeros in
A_and_Aᵀ(but the correct sparsity pattern).I'm wondering if we should store a different matrix than
b = A_and_AᵀinStarSetResultorTreeSetResult.If I remember correctly, we want to store
Bto have the correct type for the functiondecompress, but in this case, it’s always aSparseMatrixCSC, whereasAcould have a different type.We probably only want the
AdjacencyGraphofA_and_Aᵀand not the matrix itself, and we could likely achieve this with a keyword argument forAdjacencyGraphor another constructor that takes two arguments (AandAᵀ).Off-topic: We can modify the function
neighbors(g::AdjacencyGraph, v::Integer)to not usefilterin the case of bicoloring because we know that we don't have diagonal coefficients.