-
Notifications
You must be signed in to change notification settings - Fork 167
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
Sparse algebra support with OOP API #760
Conversation
Wouldn't it be useful if |
This could be done even shorter with a |
Agreed, |
Is it a good idea to have matvec product that do not initialize the result vector? I have to say that I have spent a good hour debugging a code and the source of divergence in the solver was the fact that I was not initializing the result vector before passing it to the subroutine. While I understand that this is a great way to perform Axpy operation, I would advise to have separate matvec (with initialization) and Axpy (without) routines.
|
Hi @ftucciarone, thanks for checking out the current draft and sorry for the inconvenience. Yes, I have intentionally designed the API to not initialize the vector, in the current version within FSPARSE https://github.com/jalvesz/FSPARSE?tab=readme-ov-file#sparse-matrix-vector-product I updated the README to be clear about it but forgot to update here. The reason for that choice is because it allows maximum flexibility for preprocessing linear systems needing operations with the same matrix operator that will be used for the solver. So it basically places the "responsability" on the user to place a This is of course totally open to debate, I just proposed following my experience reworking solvers. My feeling is that doing that can avoid having to manage different implementations or optionals, when the solution can be to explicitly state that the interface is updating (y = y + M * x) instead of overwriting (y = M * x). |
Hi @jalvesz, it was actually a pleasure to look into this draft, I am learning a lot and I look forward to discuss it with you if possible. I have to say that I have particularly strong feelings about the "non-initialization" problem, mostly twofold. However, I think that separating matvec and Axpy could be feasible at this stage (I can take care of that with the correct guidance) and potentially doable by preprocessing as well (I'm thinking at a fypp if). Cheers, Francesco Edit: example of splitting matvec and matAxpy with fypp #:include "../include/common.fypp"
#:set RANKS = range(1, 2+1)
#:set KINDS_TYPES = REAL_KINDS_TYPES
#:set OPERATIONS = ['matvec', 'matAxpy'] <--- DEFINE THE NAMES
#! define ranks without parentheses
#:def rksfx2(rank)
#{if rank > 0}#${":," + ":," * (rank - 1)}$#{endif}#
#:enddef
!! matvec_csr
#:for k1, t1 in (KINDS_TYPES)
#:for rank in RANKS
#:for idx, opname in enumerate(OPERATIONS) <--- ITERATE OVER THE NAMES
subroutine ${opname}$_csr_${rank}$d_${k1}$(vec_y,matrix,vec_x)
type(CSR_${k1}$), intent(in) :: matrix
${t1}$, intent(in) :: vec_x${ranksuffix(rank)}$
${t1}$, intent(inout) :: vec_y${ranksuffix(rank)}$
integer :: i, j
#:if rank == 1
${t1}$ :: aux
#:else
${t1}$ :: aux(size(vec_x,dim=1))
#:endif
associate( data => matrix%data, col => matrix%col, rowptr => matrix%rowptr, &
& nnz => matrix%nnz, nrows => matrix%nrows, ncols => matrix%ncols, sym => matrix%sym )
if( sym == k_NOSYMMETRY) then
do concurrent(i=1:nrows)
do j = rowptr(i), rowptr(i+1)-1
#:if idx==0 <--- IF MATVEC
vec_y(${rksfx2(rank-1)}$i) = data(j) * vec_x(${rksfx2(rank-1)}$col(j))
#:else <--- IF AXPY
vec_y(${rksfx2(rank-1)}$i) = vec_y(${rksfx2(rank-1)}$i) + data(j) * vec_x(${rksfx2(rank-1)}$col(j))
#:endif
end do
end do
else if( sym == k_SYMTRIINF )then
do i = 1 , nrows
aux = 0._${k1}$
do j = rowptr(i), rowptr(i+1)-2
aux = aux + data(j) * vec_x(${rksfx2(rank-1)}$col(j))
vec_y(${rksfx2(rank-1)}$col(j)) = vec_y(${rksfx2(rank-1)}$col(j)) + data(j) * vec_x(${rksfx2(rank-1)}$i)
end do
aux = aux + data(j) * vec_x(${rksfx2(rank-1)}$i)
#:if idx==0 <--- IF MATVEC
vec_y(${rksfx2(rank-1)}$i) = aux
#:else <--- IF AXPY
vec_y(${rksfx2(rank-1)}$i) = vec_y(${rksfx2(rank-1)}$i) + aux
#:endif
end do
else if( sym == k_SYMTRISUP )then
do i = 1 , nrows
aux = vec_x(${rksfx2(rank-1)}$i) * data(rowptr(i))
do j = rowptr(i)+1, rowptr(i+1)-1
aux = aux + data(j) * vec_x(${rksfx2(rank-1)}$col(j))
vec_y(${rksfx2(rank-1)}$col(j)) = vec_y(${rksfx2(rank-1)}$col(j)) + data(j) * vec_x(${rksfx2(rank-1)}$i)
end do
#:if idx==0 <--- IF MATVEC
vec_y(${rksfx2(rank-1)}$i) = aux
#:else <--- IF AXPY
vec_y(${rksfx2(rank-1)}$i) = vec_y(${rksfx2(rank-1)}$i) + aux
#:endif
end do
end if
end associate
end subroutine
#:endfor
#:endfor
#:endfor |
@ftucciarone thanks for the idea! I will also bring parts of a discussion I had with @ivan-pi on exactly this topic, where he brought to my attention the following:
https://psctoolkit.github.io/psblasguide/userhtmlse4.html#x18-550004 Taking all those ideas together, I could imagine using optionals to cover:
Or the default to be overwrite and an optional for addition, or simply with beta =1 Let me know your thoughts on that |
Updates: added in-place transpose (and conjugate transpose) for the spmv kernels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jalvesz for the update. LGTM as the implementation is very clean and it looks like you've addressed all previous comments. I would like this PR to be merged before new features are added, as it's becoming harder to track the new additions.
So I suggest any others that have comments/edits to please do so, and maybe we should think of merging soon if no further comments arise.
Thanks @perazz for the feedback. I did a few last cleanups and enhanced the sellc format support. On my side I also think the PR is now ready to be merged if there are no further comments. Future features/improvements could be addressed with separate PRs having this base available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR related to:
#38
#749
#189
Here I try to propose the OOP API upfront as a means to centralize data-containment format within stdlib, which I feel would be a (the) big added value. I'm migrating the proposal started here FSPARSE to adapt it to stdlib.
Current proposal design:
sparse_type
(abstract base type)COO_type
,CSR_type
,CSC_type
,ELL_type
,SELLC_type
(DTs with no data buffer)COO_?p_type
,CSR_?p_type
,CSC_?p_type
,ELL_?p_type
,SELLC_?p_type
(DTs with data buffer, where?
kind precision including complex values)op
:sparse_op_none='N'
(default),sparse_op_transpose='T'
orsparse_op_hermitian='H'