Adding support for SELL sparse matrix format #11
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
| slice_size::Int = 32, | ||
| ::Type{Ti} = Int32, | ||
| ) where {Tv,Ti<:Integer} | ||
| sparse_matrix_csc_t = convert(SparseMatrixCSC{Tv,Ti}, sparse(transpose(m))) |
There was a problem hiding this comment.
Why do we need to convert it? Isn't sparse(... already doing it?
| #sliceptr(A::DeviceSparseMatrixSELL) = A.slice_ptr | ||
|
|
||
| # KA functions | ||
| KernelAbstractions.get_backend(A::DeviceSparseMatrixSELL) = get_backend(A.nzval) |
There was a problem hiding this comment.
This should be already defined for the abstract type. Let's avoid to define specific methods if not necessary.
| ResVec<:AbstractVector{ResType}, | ||
| InputVec<:AbstractVector{InputType}, |
There was a problem hiding this comment.
Better to use DenseVector
| ResMat<:AbstractMatrix{ResType}, | ||
| InputMat<:AbstractMatrix{InputType}, |
There was a problem hiding this comment.
If you use DenseMatrix, the ambiguity error should go away.
|
Hi @AntoineBut, Thank you for contributing! I'm not an expert of this format, but eveything seems ok. There are several errors. I don't understand if the benchmark regression is related or not, but in principle you haven't touched anything. It would be great if you can align the definition of |
3 participants
Hi! Recently @gdalle told me he might use this package, but it is currently missing the SELL matrix format which we worked on a few months ago. I mostly copied over this from my previous repo, trying to match the design choices made in this repo. (More info on this format available in the CuSparse documentation)
Here are a few points I think are worth addressing before merging:
TransposeandAdjoint, and don't really have the time to do this now. Also, I don't fully understand how you did this with your for loops and@eval(but it looks clean)JET.test_package(DeviceSparseArrays; target_defined_modules = true)passes.c = A * b(butmul!(c, A, b)works just fine), I'm not used to defining new methods for Base functions, I'm sure its just a small thing missing somewhere but I couldn't find it.Finally, I recently started to play around with the idea of permuting rows in sparse matrices to "group" together rows according to the number of non-zeros. This functionality is implemented for SELL matrices but not used as I still need to figure out a clean and user friendly way to integrate the feature. For now, the constructor can permute the rows as it constructs the matrix, and use the
permfield in the struct to save the permutation that was performed. This allows the user to apply the same permutation on their other arrays. It"s not super user-friendly, it might be possible to create aPermutedwrapper similar to the howTransposeis usually managed but I haven't tried.