Move up linear solver #951
Conversation
endJunction
force-pushed
the
MoveUpLinearSolver
branch
from
ad7adb0
to
274295f
Compare
| @@ -84,7 +84,7 @@ ProjectData::~ProjectData() | |||
| { | |||
| delete _geoObjects; | |||
|
|
|||
| for(ProcessLib::Process* p : _processes) | |||
| for(ProcessLib::Process<GlobalSetupType>* p : _processes) | |||
There was a problem hiding this comment.
now it might be good to have a typedef for it. here also auto would suffice, but typedef would also work below.
There was a problem hiding this comment.
Don't get it; to be discussed.
|
General note: I just noticed that there is a problem with commit-by-commit code review: |
|
Good work so far. I've had quite some comments, but none of them addresses a fundamental issue. |
|
Maybe for such a big change it would be good, the next time, to initially add some todo-comments (what will be added, moved, removed) to the source code. This way people not directly involved might more easily get an idea of where the changes head to. |
| std::unique_ptr<BaseLib::ConfigTree> _linear_solver_options; | ||
| std::unique_ptr<typename GlobalSetup::LinearSolver> _linear_solver; | ||
|
|
||
| std::unique_ptr<typename GlobalSetup::MatrixType> _A; |
There was a problem hiding this comment.
We also need a possibility that _A, _x, _rhs could be shared by other processes.
There was a problem hiding this comment.
While this is not implemented yet, the current changes don't prohibit it. We shall see how it is possible when there are multiple weakly coupled processes, which shall not happen before March or so.
|
@endJunction 👍 This is the right way. |
endJunction
force-pushed
the
MoveUpLinearSolver
branch
from
4705b6e
to
e49fa5c
Compare
|
Addressed all open questions so far. Do not merge yet. I'll squash the 'md' (= merge down) commits to the origins before it goes to master. This way you can easier verify the changes. |
| } | ||
|
|
||
| bool solve(const double /*delta_t*/) override | ||
| bool assemble(const double /*delta_t*/) override | ||
| { | ||
| DBUG("Solve GroundwaterFlowProcess."); |
There was a problem hiding this comment.
Mismatch of the name of the method (assemble) and debug output (Solve ...) . ✅
|
@TomFischer if OK, I'll rebase and merge it. |
endJunction
force-pushed
the
MoveUpLinearSolver
branch
from
ebcf1d5
to
c192813
Compare
|
It is ok. Is there any label we could set for the case that the review is finished and the author of the PR can/shall merge down changes? Maybe this is in general a good approach?! |
The initialization is split into a generic one placed in Process and a specific one kept in GroundwaterFlowProcess.
endJunction
force-pushed
the
MoveUpLinearSolver
branch
from
c192813
to
4727502
Compare
Move up common process variables/functions.
|
OpenGeoSys development has been moved to GitLab. |
This is one step (of many following) to separate process specific functionality (like for the GroundwaterFlowProcess) from common parts of all monolithic processes. This is required for introduction of non-linear solver loop which will control the linear solver solve calls.
In this PR there is no functionality change nor their is any (but unavoidable) generalization, for example to handle multiple DOF per node. It's just moving some of the GroundwaterFlowProcess variables up into the Process class. Following the variables there are some functions.
Major changes in functions are:
There is one thing I like to point out: because the GroundwaterFlow class is derived from a template Process
thispointer is used to access Process's protected variables; I'll reduce their numbers in following PRs when more interfaces will be introduced.Update: I'v just seen that diff for the review look like a mess, so maybe commit-wise review is easier...