Symmetric Sparse Matrix Kernel Implementations about hiop HOT 7 CLOSED

see conventions for matrices: https://github.com/LLNL/hiop/blob/master/src/LinAlg/readme.md -> sparse symmetric matrix only stores upper triangle (nonzero) entries

also, since we AddToSymDenseMatrixUpperTriangle, we only need copy one "half" of the sparse matrix into the "half" of the dense matrix

from hiop.

this code from timesVec

if(iRow_[i]!=jCol_[i])
  y[jCol_[i]] += alpha * x[iRow_[i]] * values_[i];

makes up for the missing "half" of the sparse symmetric matrix

from hiop.

see conventions for matrices: https://github.com/LLNL/hiop/blob/master/src/LinAlg/readme.md -> sparse symmetric matrix only stores upper triangle (nonzero) entries

also, since we AddToSymDenseMatrixUpperTriangle, we only need copy one "half" of the sparse matrix into the "half" of the dense matrix

@cnpetra: Perhaps I am misreading the explanations in the readme file, but it seems to me there is an implied precondition on offsets in methods such as addToSymDenseMatrixUpperTriangle. You can pass offset arguments to the method such that the destination matrix becomes non-symmetric after this method is executed. From your explanation it seams the postcondition for addToSymDenseMatrixUpperTriangle is that the destination matrix remains symmetric. To satisfy this postcondition, I would think there has to be a precondition on offset arguments guaranteeing that source and destination matrix diagonals are aligned. I may very well be missing something here. Could you please help me understand this better?

from hiop.

Forgive me if this gets a little lengthy, but I wanted to be as clear as possible with these explanations as to avoid confusion.

First, I wanted to clarify what is happening with timesVec when applied to symmetric sparse matrices. Symmetric sparse matrices only store the upper triangle. As a result, when performing multiplication with timesVec, when accessing a specific entry at index (i,j), we should also recognize this entry as being duplicated at index (j,i). This cannot be done without caution however, as we must validate that we are not on the main diagonal of the matrix.

The two functions that I brought up in the issue are addToSymDenseMatrixUpperTriangle and addUpperTriangleToSymDenseMatrixUpperTriangle. Since these are implemented in hiopMatrix.hpp, they are to be implemented across all matrix implementations. Without considering specifics of implementation, these functions are adding either this, or transpose(this) to the upper triangle of a symmetric dense matrix. Since the output matrix is a symmetric dense matrix, you need to ensure that only the upper triangle of the symmetric dense matrix is modified, hence the precondition described in the documentation (@pelesh does this clarify?) requiring this to fit within the upper triangle of W.

Considering sparse symmetric matrices in particular now, the implementations of addToSymDenseMatrixUpperTriangle and transAddToSymDenseMatrixUpperTriangle are problematic as they are only performing half of the operation outlined above. While both methods successfully iterate over all of the CSR data, they do not recognize that the matrix is symmetric. As a result, instead of adding the entire sparse symmetric matrix to the upper triangle of the dense symmetric, only the upper triangle is being added. This is similarly an issue with the transpose method.

As such, the function is instead behaving more so as adding the upper (or lower if you are calling the transpose method) triangle of this (symmetric sparse matrix) to a symmetric dense matrix. To achieve the same effect as adding the entire symmetric sparse matrix to the upper triangle, you could potentially call both addSymDenseMatrixUpperTriangle and transAddSymDenseMatrixUpperTriangle one after the other, however this would add the values along the main diagonal of the symmetric sparse matrix twice to the destination matrix W.

If this behavior is still desirable, and not necessarily a bug, I would suggest that the function names are modified, along with documentation to adequately represent this. A suggested function name would be something along the lines of addUpperTriangleToSymDenseMatrixUpperTriangle, however I noticed that this is already a function.

I hope my explanations were clarifying rather than confusing 😄

from hiop.

I think we discussed and took care of this on PNNL's gitlab issue, please confirm.

from hiop.

Yes, this issue has been resolved. Thanks!

from hiop.

The fix is in bb69d56.

from hiop.