Optimized implementation for structured matrices V2 - #319
Conversation
Sorry, something went wrong.
| end | ||
| return SMC.BipartiteGraph(S1, S2) | ||
| end | ||
|
|
There was a problem hiding this comment.
@gdalle For now I just want to make sure the structured coloring algorithms working for Diagonal, Bidiagonal and Tridiagonal matrices, but it turns out we still need to make transpose to be GPU-compatiable to allow colorings and decompression. Do these implementations look resonable to you, if do, I will continue on this direction.
Sorry, something went wrong.
There was a problem hiding this comment.
See above, I think I need additional context to understand what you did here
Sorry, something went wrong.
gdalle
left a comment
There was a problem hiding this comment.
Thanks for getting this started!
Sorry, something went wrong.
| B_rcolor_indices = mod1.( | ||
| (row_shift + 1):(row_shift + maximum(row_colors(res))), length(colorscheme) | ||
| ) |
There was a problem hiding this comment.
If possible, it's better to avoid formatting changes that only add noise
Sorry, something went wrong.
| colptr::AbstractVector{Ti} | ||
| rowval::AbstractVector{Ti} |
There was a problem hiding this comment.
This is a really bad idea because it will introduce type instability throughout the coloring and decompression algorithms. Can you explain why you deem it necessary?
I hope the tests catch it, if not we need more type stability tests with JET
Sorry, something went wrong.
| silent && set_silent(model) | ||
| # one variable per vertex to color, removing some renumbering symmetries | ||
| @variable(model, 1 <= color[i=1:n] <= i, Int) | ||
| @variable(model, 1 <= color[i = 1:n] <= i, Int) |
There was a problem hiding this comment.
Same comment about formatting noise
Sorry, something went wrong.
| if VERSION >= v"1.10" || A isa Union{Diagonal,Bidiagonal,Tridiagonal} | ||
| # banded matrices not supported by ArrayInterface on Julia 1.6 | ||
| # @test color == ArrayInterface.matrix_colors(A) # TODO: uncomment | ||
| if algo isa StructuredColoringAlgorithm |
There was a problem hiding this comment.
We should also restrict this to the matrix types that ArrayInterface supports
Sorry, something went wrong.
| # banded matrices not supported by ArrayInterface on Julia 1.6 | ||
| # @test color == ArrayInterface.matrix_colors(A) # TODO: uncomment | ||
| if algo isa StructuredColoringAlgorithm | ||
| @test color == ArrayInterface.matrix_colors(A) |
There was a problem hiding this comment.
We may also want to test that decompression returns a matrix of the correct type
Sorry, something went wrong.
| function decompress!(A::Diagonal, B::AbstractMatrix, result::RowColoringResult) | ||
| color = row_colors(result) | ||
| for i in axes(A, 1) | ||
| A[i, i] = B[color[i], i] | ||
| end | ||
| return A | ||
| end |
There was a problem hiding this comment.
This will not work for a Diagonal matrix stored on GPU. My idea was that we could vectorize index accesses like we do for the CSC/CSR formats on GPU, and then do something like
diag(A) .= getindex.(B, color, 1:n)
Sorry, something went wrong.
| for R in (:Diagonal, :Bidiagonal, :Tridiagonal) | ||
| @eval function SMC.BipartiteGraph( | ||
| A::$R{T,<:CuArray}; symmetric_pattern::Bool=false | ||
| ) where {T} | ||
| return SMC.BipartiteGraph(CuSparseMatrixCSC(A); symmetric_pattern) | ||
| end | ||
| end |
There was a problem hiding this comment.
Why is this necessary? The coloring will take place on the CPU anyway so I don't see why we would need a GPU-based graph structure
Sorry, something went wrong.
| end | ||
| return SMC.BipartiteGraph(S1, S2) | ||
| end | ||
|
|
There was a problem hiding this comment.
See above, I think I need additional context to understand what you did here
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Basically #139 with new implementations