add a LUSOL functionality test #164

superwhiskers · 2024-06-14T20:47:18Z

this pull request adds a functionality test for LUSOL. there is one thing that needs to be worked out before this can actually be merged:

how do we finish implementing symmetric matrix expansion functionality? this is necessary for the integration test to work since LUSOL cannot operate on symmetric matrices stored only as a half (Add function to expand symmetric matrix stored as an upper/lower triangular to a full square matrix #155)
- currently, i've added a single method to the matrix::Coo class which expands the matrix. this is not permanent and it should be fully expanded upon prior to a merge with equivalent functionality for other matrix types

superwhiskers · 2024-06-14T20:57:07Z

oh. this also adds comments referencing stuff about extracting L/U factors. i'll remove that so this can be focused only on testing

pelesh

A good first stab. Here are some suggestions how to proceed.

resolve/matrix/Coo.hpp

resolve/matrix/MatrixHandlerCpu.cpp

resolve/matrix/Sparse.hpp

tests/functionality/testLUSOL.cpp

resolve/matrix/io.cpp

tests/functionality/testLUSOL.cpp

pelesh · 2024-06-18T12:44:06Z

@superwhiskers, I suggest you add fix for #162 to this PR.

resolve/matrix/Utilities.cpp

pelesh

There are some minor issues that I addressed in my comments.

Newly added expand() function needs unit test(s). I suggest we create MatrixConversionTest.hpp and runMatrixConversionTest.cpp in tests/unit directory and implement needed unit tests there.

resolve/matrix/Sparse.hpp

resolve/matrix/Utilities.cpp

resolve/matrix/Utilities.hpp

tests/functionality/testLUSOL.cpp

pelesh

Good progress but some more fixes are needed before this is ready to merge. I left a few comments to that end.

LUSOL test produces some type conversion warnings that need to be fixed.

resolve/matrix/Coo.cpp

resolve/matrix/Csr.cpp

pelesh · 2024-06-20T22:21:56Z

resolve/matrix/Sparse.cpp

+  int matrix::Sparse::setValueOwnership(bool owns, memory::MemorySpace memspace)
+  {


It should be plural in the function name -- values.

Suggested change

int matrix::Sparse::setValueOwnership(bool owns, memory::MemorySpace memspace)

{

int matrix::Sparse::setValuesOwnership(bool owns, memory::MemorySpace memspace)

{

i don't really see how that's any better. setValueOwnership is just as correct in a plural context

resolve/matrix/Utilities.cpp

pelesh · 2024-06-20T22:26:27Z

resolve/matrix/Utilities.hpp

+    /**
+     * @brief Expands a symmetric COO matrix stored only as a half to its full size
+     */
+    int expand(Coo&);
+
+    /**
+     * @brief Expands a symmetric CSR matrix stored only as a half to its full size
+     */
+    int expand(Csr&);
+
+    /**
+     * @brief Expands a symmetric CSC matrix stored only as a half to its full size
+     */


One thing that is important to mention here and in source comments is if you assume that the matrix is stored as upper- or lower triangular.

no assumption is made. technically, you could pass in a matrix that isn't triangular and it would still work

The function should not allow that. If the matrix is not upper- or lower-triangular, this function should return an error. If for some reason non-symmetric matrix was tagged as symmetric, that is a nasty bug, which we need to protect against.

it's specified as part of the preconditions for each of the expand functions. i can add a debug check that goes away in release builds to catch any bugs that violate it, but i don't see why this should be a general check we do, as the presence of a precondition makes it on the caller to know beforehand

tests/functionality/testLUSOL.cpp

pelesh · 2024-06-20T23:15:54Z

tests/unit/matrix/MatrixExpansionTests.hpp

+        TestOutcome cooMatrix5x5()
+        {
+          TestStatus status;


Tests needs to be documented. Also, the test should not be specific to particular matrix size.

tests/unit/matrix/MatrixExpansionTests.hpp

- implemented matvec over COO matrices - added back symmetric matrix expansion for COO matrices (temporary) - reworked integration test code from KLU into integration test code for LUSOL (in progress, does not fully work) - made minor tweaks to LinSolverDirectLUSOL - set data ownership at exit of matrix-from-file conversion to avoid data leaks - added a function to matrix::Sparse to set data ownership

- fix memory leak in `Csr` hijacking constructor - add hijacking constructor to `Coo` - remove needless use of `Sparse::setDataOwnership`/`Sparse::setValueOwnership` - move `Coo::expand` to a separate function in `Utilities.cpp` - add `Csr` expansion to `ReSolve::matrix::expand`

…and`

… on the stated value

…e/matrix/io.cpp

superwhiskers · 2024-06-21T22:04:06Z

rebased on latest develop

cameronrutherford · 2024-06-24T15:09:01Z

https://github.com/ORNL/ReSolve/blob/lusol-integration-tests/examples/CMakeLists.txt#L140 - might be good to have Lusol be used in the consumer test for testing resolve~cuda~rocm~klu+lusol

pelesh · 2024-06-24T15:36:50Z

https://github.com/ORNL/ReSolve/blob/lusol-integration-tests/examples/CMakeLists.txt#L140 - might be good to have Lusol be used in the consumer test for testing resolve~cuda~rocm~klu+lusol

I agree. To get there, we need to get LUSOL to work within a refactorization solver strategy, which is outside the scope of this PR.

pelesh · 2024-09-20T23:27:29Z

Closing in favor of #189.

superwhiskers requested a review from pelesh June 14, 2024 20:47

superwhiskers added enhancement New feature or request testing labels Jun 14, 2024

superwhiskers self-assigned this Jun 14, 2024

superwhiskers linked an issue Jun 14, 2024 that may be closed by this pull request

Add functionality test for LUSOL #158

Closed

pelesh changed the title ~~add a LUSOL integration test~~ add a LUSOL functionality test Jun 14, 2024

pelesh requested changes Jun 17, 2024

View reviewed changes

pelesh reviewed Jun 17, 2024

View reviewed changes

tests/functionality/testLUSOL.cpp Outdated Show resolved Hide resolved

pelesh reviewed Jun 18, 2024

View reviewed changes

resolve/matrix/Utilities.cpp Outdated Show resolved Hide resolved

superwhiskers marked this pull request as ready for review June 18, 2024 14:42

superwhiskers requested a review from pelesh June 18, 2024 14:42

pelesh requested changes Jun 18, 2024

View reviewed changes

superwhiskers mentioned this pull request Jun 19, 2024

Add function to expand symmetric matrix stored as an upper/lower triangular to a full square matrix #155

Closed

superwhiskers linked an issue Jun 19, 2024 that may be closed by this pull request

Add function to expand symmetric matrix stored as an upper/lower triangular to a full square matrix #155

Closed

superwhiskers removed a link to an issue Jun 19, 2024

Add function to expand symmetric matrix stored as an upper/lower triangular to a full square matrix #155

Closed

superwhiskers requested a review from pelesh June 19, 2024 17:59

superwhiskers mentioned this pull request Jun 19, 2024

Use consistent naming scheme across matrix and vector classes #167

Open

pelesh requested changes Jun 21, 2024

View reviewed changes

superwhiskers added 10 commits June 21, 2024 18:00

finish the implementation of a LUSOL functionality test

c87c32a

undo modifications to formatting not part of code added

62d8476

actually undo formatting changes in Coo.cpp

96507e1

re-add changes to Coo.cpp

141c7a1

remove .clang-format

16dfd4d

extract Coo matvec from MatrixHandlerCpu::matvec for now

04b0734

rework compute_error and add documentation comments to `matrix::exp…

28f1b76

…and`

compute nnz_expanded in matrix expansion functions instead of relying…

01dd894

… on the stated value

superwhiskers added 5 commits June 21, 2024 18:02

add tests for matrix expansion

7a36e7c

inline compute_error and use a forward-declaration for matvec

a1ad741

csc matrix expansion + fix bug in matrix expansion tests

e3907ed

adjust documentation comment style + undo some changes made to resolv…

b908396

…e/matrix/io.cpp

address review comments

5e69253

superwhiskers force-pushed the lusol-integration-tests branch from 7947a57 to 5e69253 Compare June 21, 2024 22:03

superwhiskers requested a review from pelesh June 21, 2024 22:05

address the one last unique_ptr -> raw pointer suggestion

ff22890

cameronrutherford mentioned this pull request Jun 24, 2024

Update spack_cpu_build.yaml to build ~klu+lusol #172

Merged

Update spack_cpu_build.yaml to build ~klu+lusol (#172)

88840d8

superwhiskers mentioned this pull request Jun 26, 2024

Remove duplicated code and consolidate coo2csr functionality in a single function. #173

Merged

superwhiskers mentioned this pull request Jul 5, 2024

alternate coo2csr implementation #175

Closed

adjust the wording of brief explanations for the expand functions

1f1e6f7

superwhiskers mentioned this pull request Jul 24, 2024

simple example using lusol #177

Closed

pelesh marked this pull request as draft August 17, 2024 11:19

pelesh mentioned this pull request Sep 20, 2024

Add functionality test for LUSOL #189

Merged

pelesh closed this Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a LUSOL functionality test #164

add a LUSOL functionality test #164

superwhiskers commented Jun 14, 2024 •

edited

Loading

superwhiskers commented Jun 14, 2024

pelesh left a comment

pelesh commented Jun 18, 2024

pelesh left a comment

pelesh left a comment

pelesh Jun 20, 2024

superwhiskers Jun 21, 2024

pelesh Jun 20, 2024

superwhiskers Jun 21, 2024

pelesh Jun 28, 2024

superwhiskers Jun 28, 2024 •

edited

Loading

pelesh Jun 20, 2024

superwhiskers commented Jun 21, 2024

cameronrutherford commented Jun 24, 2024

pelesh commented Jun 24, 2024

pelesh commented Sep 20, 2024

		int matrix::Sparse::setValueOwnership(bool owns, memory::MemorySpace memspace)
		{

add a LUSOL functionality test #164

add a LUSOL functionality test #164

Conversation

superwhiskers commented Jun 14, 2024 • edited Loading

superwhiskers commented Jun 14, 2024

pelesh left a comment

Choose a reason for hiding this comment

pelesh commented Jun 18, 2024

pelesh left a comment

Choose a reason for hiding this comment

pelesh left a comment

Choose a reason for hiding this comment

pelesh Jun 20, 2024

Choose a reason for hiding this comment

superwhiskers Jun 21, 2024

Choose a reason for hiding this comment

pelesh Jun 20, 2024

Choose a reason for hiding this comment

superwhiskers Jun 21, 2024

Choose a reason for hiding this comment

pelesh Jun 28, 2024

Choose a reason for hiding this comment

superwhiskers Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

pelesh Jun 20, 2024

Choose a reason for hiding this comment

superwhiskers commented Jun 21, 2024

cameronrutherford commented Jun 24, 2024

pelesh commented Jun 24, 2024

pelesh commented Sep 20, 2024

superwhiskers commented Jun 14, 2024 •

edited

Loading

superwhiskers Jun 28, 2024 •

edited

Loading