Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add LinSolverDirectLUSOL, with initial support for solving Ax = rhs #153

Merged
merged 12 commits into from
Jun 11, 2024
Merged

add LinSolverDirectLUSOL, with initial support for solving Ax = rhs #153

merged 12 commits into from
Jun 11, 2024

Conversation

superwhiskers
Copy link
Collaborator

  • adds a function for expanding symmetric matrices only stored as their half
  • adds a function for extracting coordinates of nonzeroes from matrices
  • documents a deficiency in the Csc class
  • modifies the build system to compile fortran code used in LUSOL
  • fixes some additional issues made in using index_type/real_type pervasively
  • adds a method to the logger class to extract the log level
  • adds tests on symmetric matrix expansion

@superwhiskers
add LinSolverDirectLUSOL, with initial support for solving Ax = rhs
e45b57e 
- adds a function for expanding symmetric matrices only stored as their half
- adds a function for extracting coordinates of nonzeroes from matrices
- documents a deficiency in the Csc class
- modifies the build system to compile fortran code used in LUSOL
- fixes some additional issues made in using index_type/real_type pervasively
- adds a method to the logger class to extract the log level
- adds tests on symmetric matrix expansion
@pelesh
Copy link
Collaborator

pelesh commented Jun 10, 2024

  • documents a deficiency in the Csc class

@superwhiskers, please create a separate issue rather than adding comments to the code.

@pelesh
Copy link
Collaborator

pelesh commented Jun 10, 2024

It seems that changes to CMake caused GPU builds to break. An example error message looks like:

[ 34%] Building CXX object resolve/matrix/CMakeFiles/resolve_matrix.dir/MatrixHandlerHip.cpp.o
/ccs/home/peles/src/resolve/resolve/resolve/matrix/MatrixHandlerHip.cpp:58:18: error: no matching function for call to 'rocsparse_dcsrmv_analysis'
        status = rocsparse_dcsrmv_analysis(handle_rocsparse,
                 ^~~~~~~~~~~~~~~~~~~~~~~~~
/opt/rocm-5.6.0/include/rocsparse/rocsparse-functions.h:3387:18: note: candidate function not viable: no known conversion from 'index_type *' (aka 'long *') to 'const rocsparse_int *' (aka 'const int *') for 8th argument
rocsparse_status rocsparse_dcsrmv_analysis(rocsparse_handle          handle,

I'll investigate more.

@superwhiskers
restore changes made to the files without adjusting formatting
08d7631
@superwhiskers
minor refactoring
fb10542 
- adjust index type precision to 32 bits
- add dummy implementations of `Sparse::expand()` and `Sparse::elements()` to `Csc`
- move scratch space allocation, among other things from `LinSolverDirectLUSOL::setup(...)` to `LinSolverDirectLUSOL::analyze()` and drop input matrix conversion to triplet format, instead assuming it is already correctly formatted
pelesh
pelesh requested changes Jun 11, 2024
View reviewed changes
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Needs some changes to put it in line with the rest of the Re::Solve code but nothing major.

resolve/CMakeLists.txt Outdated Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Outdated Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Outdated Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Show resolved Hide resolved
resolve/LinSolverDirectLUSOL.cpp Outdated Show resolved Hide resolved
resolve/matrix/Sparse.hpp Outdated Show resolved Hide resolved
resolve/matrix/Utilities.cpp Outdated Show resolved Hide resolved
resolve/matrix/Utilities.hpp Outdated Show resolved Hide resolved
tests/unit/matrix/LUSOLTests.hpp Outdated Show resolved Hide resolved
tests/unit/matrix/MatrixExpansionTests.hpp Outdated Show resolved Hide resolved
@pelesh pelesh added the enhancement New feature or request label Jun 11, 2024
pelesh
pelesh requested changes Jun 11, 2024
View reviewed changes
Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To manage the scope of this PR I suggest moving expand() function into a separate branch and adding this functionality through a separate pull request once we have functioning device implementation in addition to the current host implementation. See #155.

The symmetric matrix expansion to a full square matrix is not essential for LUSOL implementation. For this PR it would suffice to return an erro if the matrix is symmetric and not expanded.

@pelesh
Copy link
Collaborator

pelesh commented Jun 11, 2024

@superwhiskers, I tested this branch and everything looks good. I fixed a couple of minor build warnings in PR #156. I will merge this PR after #156 is merged.

@superwhiskers
Copy link
Collaborator Author

#156 is merged.

@pelesh pelesh merged commit a3892d9 into develop Jun 11, 2024
1 check passed
@superwhiskers superwhiskers deleted the integrate-lusol branch June 12, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pelesh pelesh pelesh approved these changes

@kswirydo kswirydo Awaiting requested review from kswirydo

Assignees

@superwhiskers superwhiskers

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@superwhiskers @pelesh