Resolve SUNDIALS Linear Solver interface - #9
Conversation
…error Co-authored-by: Nicholson Koukpaizan <72402802+nkoukpaizan@users.noreply.github.com>
pelesh
left a comment
pelesh
left a comment
There was a problem hiding this comment.
Very nice work! Tests pass on HIP backend.
There are some apparent bugs in the code related to device/host memory access and I am not sure how this code works. Please take a look at comments below.
The solver setup function does not check for failures and always declares success no matter what happens.
Once these issues are addressed, please remove debugging and commented out code and update the documentation.
| if (FACTORIZED(S)) | ||
| { | ||
| solver->refactorize(); | ||
| } | ||
| else | ||
| { | ||
| solver->analyze(); | ||
| solver->factorize(); | ||
| // Perform setup only if working with GPU | ||
| if (RESOLVE_MEMSPACE(S) != ReSolve::memory::HOST) | ||
| { | ||
| solver->refactorizationSetup(); | ||
| } | ||
| FACTORIZED(S) = SUNTRUE; | ||
| } | ||
|
|
||
| return SUN_SUCCESS; | ||
| } |
There was a problem hiding this comment.
This function is returning SUN_SUCCESS even when factorization fails.
| solver->getIterativeSolver().setTol(tol); | ||
|
|
There was a problem hiding this comment.
This will fail if iterative refinement is not set in the SystemSolver. Some guards are needed.
| vec_b.setData(N_VGetArrayPointer(b), RESOLVE_MEMSPACE(S)); | ||
| vec_x.setData(N_VGetArrayPointer(x), RESOLVE_MEMSPACE(S)); |
There was a problem hiding this comment.
Shouldn't these be N_VGetDeviceArrayPointer calls instead of N_VGetArrayPointer?
There was a problem hiding this comment.
These should be N_VGetDeviceArrayPointer calls although I'm still not sure why the unit test succeeds in spite of setData being given a host pointer instead of a device one. For now I will work on modify the test to use CUDA/HIP NVectors so that the N_VGetDeviceArrayPointer calls can be made.
|
|
||
| // Its possible this step is necessary for the ReSolve interface but all other | ||
| // linear solvers do not require this SUNLinSolSetup | ||
| SUNLinSolSetup(LS, A); |
There was a problem hiding this comment.
This additional set up being required is kind of strange. No other solver requires this step. I am not entirely sure why this is necessary since the SUNDIALS KLU solver does not require it. It might be a bug in the solver but the updated SUNLinSolSetup with checks does not return an error when this line is commented out.
|
For now, I have only tested the Heat Equation example on CPU. |
…_vector implementation to check device arrays
…NMatrix_Resolve docs to include the new getters
3 participants
This PR implements SUNLinSol using Re::Solve.
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.