ReSolve SUNDIALS Linear Solver interface and unit test - #10
Conversation
pelesh
left a comment
pelesh
left a comment
There was a problem hiding this comment.
This looks good -- almost ready to merge. A few things to consider:
- Please describe in the PR how to verify new solver. Test coverage needs some clarification.
- Similarly, adding Sphinx documentation for the code would be helpful.
- I am not sure why you are not using
SUNLINEARSOLVER_MATRIX_ITERATIVEinterface. True, the refactorizaton solver is classified as "direct" but with iterative refinement it behaves as any other iterative solver.
… add a check to return an NULL if an iterative method is set
pelesh
left a comment
pelesh
left a comment
There was a problem hiding this comment.
It looks good to me.
Please axpand a little bit on the documentation.
I'd appreciate reviews by @nkoukpaizan and @Steven-Roberts before merging.
nkoukpaizan
left a comment
nkoukpaizan
left a comment
There was a problem hiding this comment.
Looks good to me. Tested the CPU and CUDA backends.
Steven-Roberts
left a comment
Steven-Roberts
left a comment
There was a problem hiding this comment.
Overall looks good!
| } | ||
|
|
||
| /* Get Re::Solve solver */ | ||
| auto* solver = reinterpret_cast<ReSolve::SystemSolver*>(RESOLVE_CONTENT(S)->solver); |
There was a problem hiding this comment.
This appears a couple times, so it might be helpful to define a macro (or a more idiomatic C++ alternative). Also, static_cast should be better here.
| /* Currently, if an iterative method is set, return an error */ | ||
| if (solver->getRefinementMethod() == "fgmres" || solver->getSolveMethod() == "randgmres" || solver->getSolveMethod() == "fgmres") | ||
| { | ||
| std::cout << "Iterative methods are not currently supported"; |
There was a problem hiding this comment.
Doesn't follow normal SUNDIALS error handling conventions
| if (solver->getRefinementMethod() == "fgmres" || solver->getSolveMethod() == "randgmres" || solver->getSolveMethod() == "fgmres") | ||
| { | ||
| solver->getIterativeSolver().setTol(tol); | ||
| } |
There was a problem hiding this comment.
I don't think it's currently possible to enter this branch due to error checking, but if this gets supported, it may be better classified as SUNLINEARSOLVER_MATRIX_ITERATIVE type. I'm a bit concerned about using the tol argument when the type if SUNLINEARSOLVER_DIRECT, but I'll need to check on this...
There was a problem hiding this comment.
Right now it is impossible to enter this branch, but we plan on supporting the resolve iterative solvers and if those are used they will be classified as SUNLINEARSOLVER_MATRIX_ITERATIVE type.
| } | ||
|
|
||
|
|
||
| int SUNLinSolSetup_ReSolve(SUNLinearSolver S, SUNMatrix A) |
There was a problem hiding this comment.
The A argument is used for error checking, but I don't see it passed to the ReSolve::SystemSolver. Does that already have a reference to the matrix that it may (re)factorize in this function? Could A and that matrix get out of sync?
There was a problem hiding this comment.
The ReSolve::SystemSolver does have a reference to the matrix. I believe they could get out of sync and right now it is implicitly assumed that A and that matrix are the same. I will discuss this with @pelesh.
Co-authored-by: Steven Roberts <roberts115@llnl.gov>
Co-authored-by: Steven Roberts <roberts115@llnl.gov>
Co-authored-by: Steven Roberts <roberts115@llnl.gov>
…terative method check.
4 participants
This pull request contains only the interface and the unit test for the ReSolve Linear Solver interface.
The test is located in
test/unit_tests/sunlinsol/resolve. Set the cmake optionSUNDIALS_TEST_ENABLE_UNIT_TESTS=ONto build SUNDIALS with the unit tests.The test creates a random$N\times N$ $A$ matrix and $b$ vector for testing.
The tests can be run using
make test, or can be run from the command line using a custom system size by providing two arguments, the size of the system to be tested and whether timing should be printed (0 for no, 1 for yes).The test currently passes on CPU and CUDA GPU.
Following are results for the CPU and GPU tests. An NVIDIA A100 GPU was used for the tests
developbranch notmain.CHANGELOG.mdanddocs/shared/RecentChanges.rstfiles. Notice that the former is a markdown file and the latter is reStructuredText, so the formatting is slightly different.