TPSA restructure (2/3): New preconditioner structure - #7358
Conversation
svenn-t
added
the
manual:enhancement
|
jenkins build this please |
a4e6659 to
e87ba0f
Compare
|
Making this ready-to-review since #7357 was merged |
There was a problem hiding this comment.
Pull request overview
Introduces a field-split TPSA preconditioner and integrates it with the new TPSA matrix/vector framework.
Changes:
- Adds sequential and parallel five-field TPSA preconditioners.
- Adds per-block Hypre/ILU/AMG solver configuration.
- Migrates TPSA solver and linearizer integration to the new types.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists_files.cmake |
Registers new preconditioner files. |
TPSALinearSolverParameters.hpp |
Changes the default to Hypre. |
TPSALinearSolverParameters.cpp |
Updates TPSA solver parameters and help. |
setupPropertyTree.hpp |
Declares TPSA configuration setup. |
setupPropertyTree.cpp |
Builds block-solver configuration and fallback. |
tpsa/TpsaPreconditioner.hpp |
Implements the field-split preconditioner. |
tpsa/TpsaPreconditioner.cpp |
Adds explicit template instantiations. |
tpsa/TpsaPreconditionerFactory.hpp |
Registers sequential and MPI factories. |
ISTLSolverTPSA.hpp |
Integrates the new matrix and preconditioner. |
TTagFlowProblemTPSA.hpp |
Selects new TPSA matrix/vector types. |
tpsalinearizer.hpp |
Uses stable TPSA block handles. |
Suppressed comments (1)
opm/simulators/linalg/tpsa/TpsaPreconditioner.hpp:236
- The sweep is described as block lower-triangular, but the solid-pressure defect omits the stored lower-triangular rotation coupling
S[_4][_3]. For a nonzero solid-pressure/rotation block, the final solve therefore applies a different (block-diagonal in rotation/pressure) preconditioner than documented.
S_[_4][_0].mmv(v[_0], d4);
S_[_4][_1].mmv(v[_1], d4);
S_[_4][_2].mmv(v[_2], d4);
sPresSolver_->apply(v[_4], d4, result);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (conf == "hypre") { | ||
| OpmLog::warning( | ||
| "--tpsa-linear-solver=hypre requires a build with Hypre support (USE_HYPRE=ON). " | ||
| "Switching to ilu0!"); | ||
| conf = "ilu0"s; |
There was a problem hiding this comment.
removed const from conf input
| newton_use_gmres_ = false; | ||
| ignoreConvergenceFailure_ = false; | ||
| linsolver_ = "ilu0"; | ||
| linsolver_ = "default"; |
There was a problem hiding this comment.
thought I'd purged this... Changed to hypre
| dispSolver0_->apply(v[_0], d0, result); | ||
| dispSolver1_->apply(v[_1], d1, result); | ||
| dispSolver2_->apply(v[_2], d2, result); |
There was a problem hiding this comment.
All istl solvers set v = 0.0 before apply(), but added a v = 0.0 inside apply() just in case...
| * \param v Update, overwritten by the result | ||
| * \param d Defect to precondition | ||
| */ | ||
| void apply(MultiVector& v, const MultiVector& d) override |
There was a problem hiding this comment.
added a new unit test for TpsaPreconditioner. Just apply() tests for now. The block S[_4][_3] is not used in preconditioner. It might be zero anyways (needs further investigation...)
| ("Configuration for linear solver. Valid preset options are: hypre, ilu0, dilu, " | ||
| "or amg. Alternatively, you can request a configuration to be read from a JSON file by " | ||
| "giving the filename here, ending with '.json.'"); |
e87ba0f to
1b09f7a
Compare
| module for the precise wording of the license and the list of | ||
| copyright holders. | ||
| */ | ||
| #include <config.h> |
There was a problem hiding this comment.
i know this is inherited faulty structure, but unless the .hpp only contains the template definitions, and the implementations are in a _impl.hpp file (which is only included in this file), this .cpp file is basically just additional work for no gain. it will still instantiate and compile the implementation in all TUs using it, with the linker deduplicating it in the end. have a look at e.g. Transmissibility(_impl).[hc]pp if it's unclear what I mean.
There was a problem hiding this comment.
I think I see what you mean, but before I do some changes I want to confirm: If I split the implementation out to an _impl.hpp file and use that one in the cpp file, then it will only be compiled once?
There was a problem hiding this comment.
yes. this serves two goals;
the .cpp file contains the explicit instantiations for the chosen combinations, and downstream code relying on those instantiations can just include the .hpp file and they will share instantiations (ie they do not see the implementations of the templates at all, only the declaration).
however, should a downstream code need some other instantiation, they can explicitly include the _impl.hpp file, the whole template definition is then available to them and they can instantiate for other types.
you can see this pattern being used in e.g. flow_blackoil_alugrid.cpp. Transmissibility.cpp only contains instantiations for CpGrid. Instead of using Transmissibility.hpp we include the Transmissibility_impl.hpp so we can instantiate for Alugrid.
There was a problem hiding this comment.
Thanks for the explanation! It has not been clear to me the reason for _impl files, just thought it was purely a code style thing, but now I see the point! I will update the PR with an _impl file for TpsaPreconditioner.
- One preconditioner for whole TPSA system; three sub-block preconditioner can be set by user - Allows for Hypre, which is default if linked; else ilu0
1b09f7a to
1aaed7e
Compare
Use the new matrix and vector framework for TPSA to make a new preconditioner structure, following the same principles as the SystemPreconditioner for flow. This enable the use of separate preconditioners on the diagonal blocks of the TPSA system matrix. New default solver is Hypre, with fallback to ILU0 if not installed.
Includes PR #7357, which should be merged first. Draft mode until then.