Dense Matrix Test Consolidation #161
Conversation
…NumLocRows, getNumLocCols in matrixTests.hpp
| const local_ordinal_type M = A->m(); | ||
| const local_ordinal_type N = A->n(); | ||
| int fail = 0; | ||
| const real_type* amat = getLocalDataConst(A); |
There was a problem hiding this comment.
Using getLocalDataConst here seemed to be appropriate. The only way this implementation breaks is if we move away from using a single array to store matrix data (both RAJA and non-RAJA matrices), or if distributed matrices end up being implemented.
Since distributed matrices would be tested independently from local dense matrices (perhaps I am wrong in this assumption), I think that this is a fine solution.
| } | ||
|
|
||
| /// Checks if _local_ vector elements are set to `answer`. | ||
| int verifyAnswer(hiop::hiopVector* x, real_type answer) |
There was a problem hiding this comment.
This and the following vector verifyAnswer methods are directly copied from the vector tests, along with any necessary helper methods.
| virtual real_type getLocalElement( | ||
| const hiop::hiopVector* x, | ||
| local_ordinal_type i) = 0; | ||
| virtual local_ordinal_type getNumLocRows(hiop::hiopMatrixDense* a) = 0; |
There was a problem hiding this comment.
I have removed getNumLocRows, getNumLocCols and getLocalSize methods, replacing them with the m(), n() and get_local_size() methods respectively, to avoid separate implementations.
There was a problem hiding this comment.
We need to keep methods that return local numbers of rows and columns as unit test helpers, because it is not certain all matrix implementations will have such public methods.
Furthermore, this change introduces a bug as m() and n() are not local matrix sizes.
There was a problem hiding this comment.
Sounds good. I will revert this change and keep the helper methods
| * | ||
| */ | ||
| #include <hiopMatrixDenseRowMajor.hpp> | ||
| #include <hiopVectorPar.hpp> |
There was a problem hiding this comment.
This is needed to implement the getLocalDataConst method for vectors, which in turn allows for the verifyAnswer method to be implemented for vectors.
If we want to test non-RAJA dense matrices with RAJA vectors and vice versa, this may need to be reconsidered, but I do not think this is necessary and so I think this is ok.
| * maps to local indices, otherwise false and does not alter | ||
| * local coordinates. | ||
| */ | ||
| virtual bool globalToLocalMap( |
There was a problem hiding this comment.
I could not find a place where this function was used, and so I removed it. I can easily re-implement if necessary, but I assumed that distributed matrices were not going to be tested under the same testing framework as dense matrices.
There was a problem hiding this comment.
I think this function was a stillborn. I don't think we ever used it.
| fail += test.matrixTimesMat(*A_mxk_nodist, *A_kxn_nodist, *A_mxn_nodist); | ||
| fail += test.matrixAddDiagonal(*A_nxn_nodist, *x_n_nodist); | ||
| fail += test.matrixAddSubDiagonal(*A_nxn_nodist, *x_m_nodist); | ||
| //fail += test.matrixAddToSymDenseMatrixUpperTriangle(*A_nxn_nodist, *A_mxk_nodist); |
There was a problem hiding this comment.
Do we want to be testing matrixAddToSymDenseMatrixUpperTriangle? Currently, the test is not implemented
Yes, please reverse those changes. The objective of this PR is to simplify unit test helper methods, but not at the expense of reducing the functionality of unit tests. |
|
Let's discuss this more before we create too much extra work for ourselves. |
Fixing test failure when not installed Closes #161 See merge request exasgd/frameworks/exago!133
Since #141 has been merged, we can start working on Dense Matrix refactoring to continue working on #133.
I tried to replicate many of the changes made in #141 here, but welcome any suggestions found in the review of this PR.