[WIP] Add FemSolverBackend and AMGXSolverBackend. #511
Conversation
|
@andlaus: Hello Herr Dr. andlaus, could you take a look a the part in fvbasdiscretization.hh and sugest a better way to select the different sparse matrix adapter options and solvers? |
|
@andlaus: PS: no bike shedding plz ;-) |
|
Generally, I'm in favor of this. When it comes to the concrete implementation, I'm wondering quite a bit if it is really necessary that so many files which have nothing to do with the linear solver are affected... (I haven't yet looked at the code in depth, or tried to use it, though.) about the linear solver selection mechanism: create a new type tag for each of these classes, set all the Properties which you need on this type tags and finally splice the respective one into your problem's type tag using the type tag and property definition: https://github.com/OPM/ewoms/blob/master/ewoms/linear/parallelamgbackend.hh#L50-L66 for the nitpicking: I promise not to care about coding style consistency during review of this PR if you vow to support the subsequent cleanup PR... |
|
So dune-fem is mandatory to use any of this? |
|
@blattms: dune-fem contains a couple of solver interface that include bindings for PETSc. dune-fem was already an optional dependency for ewoms, so that seemed like a good solution to avoid re-coding the complete bindings. These bindings to PETSc allow to access solvers provided by PETSc but are also needed for the AMGXWrapper (https://github.com/barbagroup/AmgXWrapper) which provides an interface to AMGX using PETSc. I think eventually this wrapper will become part of PETSc. In addition, the use of this solvers allowed to design the SparseMatrixAdapter in ewoms and flow to allow for different implementations of sparse matrix data structures and linear solver packages without changing the assembly code and without copying of matrices. Parts of these developments are already merged and with this PR I'm just putting out the rest that was missing. |
|
that is a lot of depencndencies (dune-fem, AMGXWrapper, Petsc, AMGX) just to use AMGX. Probably too much for me. |
|
Yes, AMGX is still quite painful to get going. We have a dialogue with Nvidia on it, hopefully they can help us make it easier to use AMGX. The build issues are challenging at this point. I suggest we do a video meet-up to discuss a way forward. I am very grateful or all the work done in this PR, it is a very good starting point for exploring the potential in GPUs. |
I agree. That is why this is and should stay an optional dependency. |
dr-robertk
force-pushed
the
PR/fem-solver-backend
branch
from
22c5906
to
60e877c
Compare
dr-robertk
changed the title
|
This will be very good!! Merge it as soon as possible so we do not miss it. Together with the more flexible dune interface and good? cpr implementations this will make linear solver possibilities much more complete. I would be interested in setting up the similar cpr preconditioner in petsc for comparison... |
|
jenkins build this serial please |
|
The tests fail because "ebos/femcpgridcompat.hh" no longer exists in ewoms. The ebos dir was moved to opm-simulators some time ago. Perhaps that file should be moved back to ewoms (evidently it is not only used for ebos/flow)? Or maybe another solution is more appropriate? |
dr-robertk
force-pushed
the
PR/fem-solver-backend
branch
from
60e877c
to
47e32d5
Compare
|
@atgeirr: I removed the ebos/femcpgridcompat.hh since the code seems to be compiling without it. |
|
jenkins build this serial please |
ewoms/nonlinear/newtonmethod.hh
Outdated
| @@ -371,9 +371,12 @@ public: | |||
| asImp_().linearizeAuxiliaryEquations_(); | |||
| linearizeTimer_.stop(); | |||
|
|
|||
| // finalize the linearization process if not done already | |||
| linearizer.finalize(); | |||
There was a problem hiding this comment.
This looks like a potentially important change?
There was a problem hiding this comment.
For the ISTL backend this calls compress of matrix if implicit build mode is used and for PETSc it calls finalize assembly. For the current use of ISTL this function does nothing.
| typedef Dune::Fem::ISTLLinearOperator < DiscreteFunction, DiscreteFunction > LinearOperator; | ||
| #endif | ||
|
|
||
| struct FemMatrixBackend : public LinearOperator |
There was a problem hiding this comment.
It is odd to place this code in the fvbasediscretization.hh file, why is it here?
There was a problem hiding this comment.
I agree. What is a better place and way to select the different solvers?
There was a problem hiding this comment.
What is a better place and way to select the different solvers?
the problem, with type tag splices. instructions.
|
ok: I tried to give this a whirl, but I failed miserably because it seems like a lot of dune-fem infrastructure is simply not there in the 2.6 branch. does this only work for the latest DUNE and dune-fem masters? |
|
@andlaus: Yes everything master at the latest version. You could have asked but I guess it was to much to ask for ;-) |
dr-robertk
force-pushed
the
PR/fem-solver-backend
branch
from
47e32d5
to
063bbba
Compare
|
@atgeirr: I fixed the linearizer.finalize(); which actually should go inside the linearizer timer section. |
|
jenkins build this serial please |
|
@andlaus: Like the ISTLMatrixBackend where should I put the code from fvbasediscretization to make the changes in fvbasediscretization only a few lines? |
okay, before this can be merged, it needs to compile with Dune 2.5 and 2.6, IMO. (it needs to compile but does not necessarily need to work with them.)
I think the If you want, I can provide you with a patch to that end. |
|
It should build with 2.5 and 2.6, I tested 2.5 and the build worked yesterday. So don't know what went wrong on your side. |
does it also compile if dune-fem is detected by the build system and if some tests activates the dune-fem solver backend if it is available? |
|
So you are asking whether the code compiles with the 2.5 release of dune-fem when it needs things that are only implemented in the current master. Without testing I can answer: NO! But I may need to add some version checks here and there. |
|
no, I'm asking if if compiles if (a) dune and dune-fem are available and on a release branch which we ought to support and (b) some test in eWoms has been modified to use the new fem linear solver backend. in the case of (b) the test should probably fall back to the standard backend if dune-fem is not on master... |
|
Ok maybe I make it separate: The fem-solver backend needs the dune-fem master. |
|
This means that using dune-fem 2.5 and wanting to use the fem solver backend will not work. Does this answer your question? |
|
So to conclude: I will add some DUNE_VERSION... checks to make clear, which version is to used. |
|
I mean that I want grid apdaptivity to continue to work with dune-2.5 and I want some test to exercise the new dune-fem linear solver code if dune-fem > 2.6 is available.
yes, that will do the trick... |
|
Just to confirm, the tests compile with with dune 2.5 and dune-fem found without using the fem solvers. |
ok, but it seems like there is nothing in |
|
Yes, because first I wanted to get all the changes in place without disturbing the existing stuff. Once that is done I will introduce an example. |
|
I do not want dangling code in the repository because past experience has shown that it always bit-rots. (@akva2: how much effort would it be to do nightly coverage runs on jenkins?) anyway, modifying one of the tests to use each of the new backend if it is available should not be too much effort if you go with the splicing mechanism. |
|
okay, I fixed most of the issues I'm bugged with in this PR in dr-robertk#1. Note that the Petsc and AMG-X backends almost certainly will not work with that. (I have little motivation in getting these prerequisites working.) |
|
During doing dr-robertk#1, I've noticed that these new backends currently produce a shipload of compiler warnings on GCC 9.1.1. (No, I did not enable masochistic warning flags.) These need to be fixed before merge. |
dr-robertk
force-pushed
the
PR/fem-solver-backend
branch
from
1099ccb
to
76e4371
Compare
|
@andlaus coverage (gcov) is run in the static analysis jobs now, e.g. https://ci.opm-project.org/job/opm-grid-static-analysis/ |
|
@blattms: I would like this to be merged before the release, if possible. I'll fix the conflicts. |
|
In that case you would need to resolve the merge conflicts, make the tests pass and get it merged into master. (Seem like quite some work to me.) Judging from the conversation it seems that both Petsc and AMGXSolver will only work if DUNE < 2.6 (i.e. master) including dune-fem is available. Is that correct. |
|
I actually did that already, just the PETSc stuff. It's in a different branch. I'll try to fix this on Wednesday. |
That might very well be the case, but there is no PR for it or I could not find it. |
dr-robertk
force-pushed
the
PR/fem-solver-backend
branch
from
4526060
to
f7668dc
Compare
|
I pushed the updated version. I'll assess whether it's a good idea to put this into the release or not. |
|
@blattms: There is a problem with some parameters and I won't get this ready until the release. So lets do it afterwards. |
This PR adds a FemSolverBackend for using linear solvers implemented in dune-fem, including everything PETSc has to offer and an AMGXSolverBackend for the AMGXSolver.