Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use SuiteSparseGraphBLAS's sparsematrix implementation (GBMatrix) #19

Closed
wants to merge 2 commits into from

Conversation

CarloLucibello
Copy link
Member

@CarloLucibello CarloLucibello commented Sep 8, 2021

This uses SuiteSparseGraphBLAS.GBMatrix as a replacement for SparseArryas.SparseMatrixCSC.
It looks like many features are missing in GBMatrix though and a ton of tests are not passing.

@CarloLucibello CarloLucibello marked this pull request as draft September 22, 2021 10:33
@rayegun
Copy link

rayegun commented Nov 16, 2021

Can you assign this to me @CarloLucibello?

@CarloLucibello
Copy link
Member Author

Sure, thanks! I rebased on master and I guess you should be able to push to this branch now

@CarloLucibello
Copy link
Member Author

Ideally, I'd like any sparse matrix backend to act as a drop-in replacement of SparseMatrixCSC, so that it should be possible to easily switch between backends and compare performance. We could even consider introducing a new graph_type = :sparseblas.

Then one can look into specializing propagate on specific operations and graph types, as done here
https://github.com/CarloLucibello/GraphNeuralNetworks.jl/blob/5d53d0585c319e5f7dc574e2e393e52e8473146a/src/msgpass.jl#L157

Unfortunately, I think decent gpu support won't be available for a while for any sparse matrix type (although things are moving a bit in CUDA.jl)

@@ -124,7 +124,7 @@ function to_sparse(A::ADJMAT_T, T::DataType=eltype(A); dir=:out, num_nodes=nothi
A = T.(A)
end
if !(A isa AbstractSparseMatrix)
A = sparse(A)
A = GBMatrix(sparse(A))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A = GBMatrix(sparse(A))
A = GBMatrix(A)

?

@rayegun
Copy link

rayegun commented Nov 16, 2021

I'll have a chance to look at this later in the week. I'll check with the upstream author about the status of the GPU portion, but it'll take a bit for sure.

@CarloLucibello
Copy link
Member Author

I'll check with the upstream author about the status of the GPU portion, but it'll take a bit for sure.

it doesn't have to come all together. Actually I would keep the two things separate: first PR we integrate cpu GBMatrix support, then we see what we can do about gpus

@CarloLucibello
Copy link
Member Author

SuiteSparse's license seems complicated
https://github.com/DrTimothyAldenDavis/SuiteSparse/blob/master/LICENSE.txt
Is this compatible with our MIT license?

@rayegun
Copy link

rayegun commented Nov 16, 2021

We're only using the GraphBLAS part which is compatible (and pretty much separate) IIRC it's Apache 2. SuiteSparse's many other parts are mostly GPL, but we're safe, they are compiled separately and not linked.

I did accidentally include SuiteSparse.jl as a dependency in my most recent commit to #master so this is a good remind :).

@CarloLucibello
Copy link
Member Author

closing in favor of #222

@CarloLucibello CarloLucibello deleted the cl/graphblas branch July 18, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants