-
Notifications
You must be signed in to change notification settings - Fork 297
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.
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
[ODESolver] Explicit link to linear solver #4628
[ODESolver] Explicit link to linear solver #4628
Conversation
[ci-build][with-all-tests] |
1 similar comment
[ci-build][with-all-tests] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this touch directly the ODE solving, did you run some time benchmarks to see if there is a change of speed ?
Why add a diamond inheritance instead of modifying directly the common parent class ODESolver ? |
Because explicit solvers do not necessarily require a linear solver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely 👍 the PR, I have some comments though.
To be discussed at SOFA dev tomorrow
Sofa/framework/Simulation/Core/src/sofa/simulation/MechanicalOperations.cpp
Show resolved
Hide resolved
Sofa/Component/ODESolver/Backward/src/sofa/component/odesolver/backward/EulerImplicitSolver.h
Show resolved
Hide resolved
6e0d62d
to
f8114b9
Compare
Sofa/Component/ODESolver/Forward/src/sofa/component/odesolver/forward/EulerSolver.cpp
Show resolved
Hide resolved
if (!l_linearSolver.get()) | ||
{ | ||
l_linearSolver.set(getContext()->get<LinearSolver>()); | ||
|
||
if (!l_linearSolver) | ||
{ | ||
msg_error() << "A linear solver is required by this component but has not been found."; | ||
this->d_componentState.setValue(sofa::core::objectmodel::ComponentState::Invalid); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also make this as a callback
Sofa/framework/Simulation/Core/src/sofa/simulation/MechanicalOperations.cpp
Show resolved
Hide resolved
f8114b9
to
f633fd1
Compare
It basically removes the use of
MultiMatrix
which hides the use of aLinearSolver
. Implicitly, the first linear solver found was used. Now, theLinearSolver
can be defined directly in the ODE solver as a Link.By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if