Add LinearSolver interface#489
Conversation
* Add rosenbrock * Add rosenbrock tests * Add RODAS5P tableau * Add rodas5p order test * Update Rosenbrock to use new LinearSolver interface * Moved Rosenbrock support classes to Native folder * Change Integrator namespace * Remove ResolveMemoryUtils.hpp --------- Co-authored-by: alexander-novo <alexander-novo@users.noreply.github.com> Co-authored-by: Shaked Regev <35384901+shakedregev@users.noreply.github.com> Co-authored-by: pelesh <peless@ornl.gov>
pelesh
left a comment
There was a problem hiding this comment.
Brief comments on interfacing with Re::Solve
| if (reuse_factors) | ||
| { | ||
| if (int err = lin_solver_.refactorize()) | ||
| return err; | ||
| } |
There was a problem hiding this comment.
I believe there should be a third option reuse_pivot_sequence. Then, this conditional would look something like this:
| if (reuse_factors) | |
| { | |
| if (int err = lin_solver_.refactorize()) | |
| return err; | |
| } | |
| if (reuse_factors) | |
| { | |
| return 0; | |
| } | |
| else if (reuse_pivot_sequence) | |
| { | |
| if (int err = lin_solver_.refactorize()) | |
| return err; | |
| } |
Might be a good idea to have an enum instead of multiple boolean flags.
| ReSolve::vector::Vector resolve_rhs(rhs.getSize()); | ||
| ReSolve::vector::Vector resolve_lhs(lhs.getSize()); | ||
|
|
||
| if (int err = resolve_rhs.setData(rhs.getData(), memspace_)) | ||
| return err; | ||
|
|
||
| if (int err = resolve_lhs.setData(lhs.getData(), memspace_)) | ||
| return err; | ||
|
|
||
| if (int err = lin_solver_.solve(&resolve_rhs, &resolve_lhs)) | ||
| return err; |
There was a problem hiding this comment.
I really don't like setData methods as they are ticking bomb but we still carry them along. How about we create a view constructor to make a shallow copy of the vector:
| ReSolve::vector::Vector resolve_rhs(rhs.getSize()); | |
| ReSolve::vector::Vector resolve_lhs(lhs.getSize()); | |
| if (int err = resolve_rhs.setData(rhs.getData(), memspace_)) | |
| return err; | |
| if (int err = resolve_lhs.setData(lhs.getData(), memspace_)) | |
| return err; | |
| if (int err = lin_solver_.solve(&resolve_rhs, &resolve_lhs)) | |
| return err; | |
| ReSolve::vector::Vector resolve_rhs(rhs.getSize(), rhs.getData()); | |
| ReSolve::vector::Vector resolve_lhs(lhs.getSize(), lhs.getData()); | |
| if (int err = lin_solver_.solve(&resolve_rhs, &resolve_lhs)) | |
| return err; |
Even better would be if that constructor is made private and we implement method createVectorView that would invoke such constructor and create vector object that does not own the data.
pelesh
left a comment
There was a problem hiding this comment.
This is good to merge. I suggest two minor changes before we merge it:
- Have
Rosenbrockclass inherit fromDynamicSolverclass. - Rebase to the current
develop.
| : lin_solver_(lin_solver), memspace_(memorySpaceAsResolve(memspace)) | ||
| { |
There was a problem hiding this comment.
You really don't need this. The SystemSolver class will manage host/device memory spaces under the hood.
| : lin_solver_(lin_solver), memspace_(memorySpaceAsResolve(memspace)) | |
| { | |
| : lin_solver_(lin_solver) | |
| { |
In the PR the memspace_ is used only to set vector/matrix views. I would just mark those places as @todo to remove them when we improve linear algebra support in GridKit. Better to merge this PR now and address these issues in the next PR.
| class Rosenbrock | ||
| { | ||
| using RealT = typename GridKit::ScalarTraits<ScalarT>::RealT; |
There was a problem hiding this comment.
Rosenbrock class should inherit from DynamicSolver.
| that the use of Enzyme is experimental, and some versions of it have been found to break GridKit code. | ||
| - [LLVM](https://github.com/llvm/llvm-project) >= 15.x. GridKit is | ||
| currently tested with LLVM 16. | ||
| - [ReSolve](https://github.com/ORNL/ReSolve) == commit `a93fb6571e3542b2fa779983fef0186a034c53c9` (optional) |
There was a problem hiding this comment.
We are about to release v0.99.3 so this is probably better dependency requirement.
| - [ReSolve](https://github.com/ORNL/ReSolve) == commit `a93fb6571e3542b2fa779983fef0186a034c53c9` (optional) | |
| - [ReSolve](https://github.com/ORNL/ReSolve) >= 0.99.3 (optional) |
Description
Add new
LinearSolverinterface and one implementation using Re::Solve to be used by e.g. Rosenbrock (#479)Proposed changes
Interface has three lifecycle functions:
configureSolvercalled whenever the matrix changes sparsity patternsetupSolvercalled whenever the matrix changes datasolveto perform a linear solveChecklist
-Wall -Wpedantic -Wconversion -Wextra.Please see Rosenbrock tests for unit tests.
Further comments