Unify linear solvers #2848

atgeirr · 2020-10-10T08:26:46Z

This is the last (I hope) take on this, replacing #2706. There are still two requests @blattms made in that PR that need to be addressed here:

If we skip --use-cpr like in this PR then to the very least I would suggest having "cpr" here.
But "--linear-solver-configuration=cpr" is stil a lot more to type than "--use-cpr"

Part of my goal was to get rid of parameters that were redundant or came into conflict with each other. Adding "cpr" to the set of strings accepted by "--linear-solver-configuration" is no problem, just not sure if we should choose the trueimpes or quasiimpes as default (probably trueimpes, as that is what you get with --use-cpr right now). However keeping the "--use-cpr" will then conflict with the other one potentially. Would it address your concern if we shortened "--linear-solver-configuration" to "--linear-solver" or even "--linsolver"?

How do I select AMG as preconditioner (formerly --use-amg), I think that proved good for some 2-phase problems of @totto82?

I think adding the option string "amg" to the above-discussed parameter (and an associated setup like for "ilu0" and the "cpr_*" options) should handle this. Could you advise about the actual case that benefited from it @totto82 or @blattms? Would be good to verify that the new is no worse.

The replacement consists of using the FlexibleSolver code.

When using the ParallelOverlappingILU0 it is not necessary to fix the overlap rows.

atgeirr · 2020-10-10T08:27:07Z

jenkins build this serial please

atgeirr · 2020-10-10T08:34:18Z

benchmark please

ytelses · 2020-10-10T10:58:33Z

Benchmark result overview:

Test	Configuration	Relative
opm-git	OPM Benchmark: flow_mpi_extra - Threads: 1	1.006
opm-git	OPM Benchmark: flow_mpi_extra - Threads: 8	1.005
opm-git	OPM Benchmark: flow_mpi_norne - Threads: 1	0.997
opm-git	OPM Benchmark: flow_mpi_norne - Threads: 8	0.992

Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

View result details @ https://www.ytelses.com/opm/?page=result&id=922

alfbr · 2020-10-10T11:11:37Z

Please note that the variance in results have been significantly reduced, performance differences at the 1% level should be detectable now. In this case, it is clear that there are no significant performance impact of this PR,

opm/simulators/linalg/FlowLinearSolverParameters.hpp

bska

I think I'd personally write the shouldCreateSolver() function using early returns like below, but it's a style issue so I'm not going to insist. Otherwise this looks good to me.

bool shouldCreateSolver() const
{
    // Decide if we should recreate the solver or just do
    // a minimal preconditioner update.
    if (!flexibleSolver_) {
         return true;
    }

    if (this->parameters_.cpr_reuse_setup_ == 0) {
        return true;
    }

    if (this->parameters_.cpr_reuse_setup_ == 1) {
        // Recreate solver on the first iteration of every timestep.
        return this->simulator_.model().newtonMethod().numIterations() == 0;
    }

    if (this->parameters_.cpr_reuse_setup_ == 2) {
        // Recreate solver if the last solve used more than 10 iterations.
        return this->iterations() > 10;
    }

    // Never recreate solver.
    assert(this->parameters_.cpr_reuse_setup_ == 3);

    return false;
}

atgeirr · 2020-10-11T06:09:34Z

write the shouldCreateSolver() function using early returns

I'll adopt your suggestion.

atgeirr · 2020-10-11T08:02:37Z

The following changes have been made:

Make the changes suggested by @bska.
Refactor setupPropertyTree() and add new option "amg" to --linear-solver-configuration, as requested by @blattms.

I have tested the amg option on Norne, and it seems to work, although slower than either "ilu0" (default) or "cpr_trueimpes" for that case. It could probably benefit from some tuning to be as good as possible for the two-phase cases mentioned where it was good before, such testing should be very easy using the JSON file approach (start testing from the tree printed in the DBG file) with no recompilation. When the best defaults are found, they can be changed in setupPropertyTree.cpp.

The one change I have not made (yet) is changing the parameter name --linear-solver-configuration. I am tempted to change it to --linsolver, but that can be done in a separate PR.

atgeirr · 2020-10-11T08:04:24Z

jenkins build this serial please

bska

Thanks for the updates to shouldCreateSolver. I just noticed a couple of problems with the new logic, but otherwise it looks good to me.

opm/simulators/linalg/ISTLSolverEbos.hpp

atgeirr · 2020-10-11T21:10:17Z

jenkins build this serial please

blattms

Please note, that with the deletion of some of the files we might also be loosing some implemented functionality. Not sure whether this is intended (because they proofed to be bad/not working) or not, but it might be hard to recover it:

BlackoilAMG had the option to start the second step on the first coarse level pressure system (now it will be only the finest one).
I think some different scaling techniques to recover the pressure system might be lost (or were already lost previously).

There has been a vast amount of refactoring. Please make sure that proper credit is given and that copyright for copied/refactored stuff is correct.

blattms · 2020-10-12T09:37:39Z

opm/simulators/linalg/FlowLinearSolverParameters.hpp

-            EWOMS_REGISTER_PARAM(TypeTag, int, CprReuseSetup, "Reuse Amg Setup");
-            EWOMS_REGISTER_PARAM(TypeTag, std::string, LinearSolverConfiguration, "Configuration of solver valid is: ilu0 (default), cpr_quasiimpes, cpr_trueimpes or file (specified in LinearSolverConfigurationJsonFile) ");
+            EWOMS_REGISTER_PARAM(TypeTag, int, CprReuseSetup, "Reuse preconditioner setup. Valid options are 0: recreate the preconditioner for every linear solve, 1: recreate once every timestep, 2: recreate if last linear solve took more than 10 iterations, 3: never recreate");
+            EWOMS_REGISTER_PARAM(TypeTag, std::string, LinearSolverConfiguration, "Configuration of solver. Valid options are: ilu0 (default), cpr_quasiimpes, cpr_trueimpes or file (specified in LinearSolverConfigurationJsonFile) ");


Needs an update as it seems to be missing options: (e.g. amg)

tests/test_blackoil_amg.cpp

blattms · 2020-10-12T10:30:52Z

jenkins build this serial please

Not sure: Does this build parallel, too?

akva2 · 2020-10-12T10:32:07Z

yes.

blattms · 2020-10-12T10:42:40Z

I have another suggestion. Why not skip the --linear-solver-configuration-file option and just intrepret --linear-solver-configuration as a file if the name matches ".*.json"?

atgeirr · 2020-10-12T12:26:12Z

I have another suggestion. Why not skip the --linear-solver-configuration-file option and just intrepret --linear-solver-configuration as a file if the name matches ".*.json"?

I like that idea. Should we also shorten it to --linsolver?

blattms · 2020-10-12T12:36:37Z

The shorter the better, if you ask me.

This is now handled by passing a string to --linear-solver-configuration that ends with ".json".

atgeirr · 2020-10-12T15:14:43Z

The shorter the better, if you ask me.

I have implemented that (--linsolver). I have also removed the separate file parameter, replacing it with passing "--linsolver=options.json" as suggested. The doc string has been updated.

There has been a vast amount of refactoring. Please make sure that proper credit is given and that copyright for copied/refactored stuff is correct.

I have added 2020 SINTEF and Equinor copyright to the ISTLSolver.hpp file. However I have not done any other additions to copyrights in the PR. Looking at the files changed by this PR, I see no other significant enough changes to warrant adding a SINTEF copyright, the rest is just removing stuff. Did you have any particular concern?

With that, I hope this can be merged when green again.

atgeirr · 2020-10-12T15:15:03Z

jenkins build this serial please

atgeirr · 2020-10-12T19:46:31Z

I made a serious mistake in one of the last commits, and it is a very embarrassing one. I'll amend ASAP...

atgeirr · 2020-10-12T19:59:52Z

jenkins build this serial please

opm/simulators/linalg/setupPropertyTree_impl.hpp

bska · 2020-10-13T10:00:56Z

opm/simulators/linalg/ISTLSolverEbos.hpp

-            {
-                prm_ = setupPropertyTree<TypeTag>(parameters_);
-            }
+            prm_ = setupPropertyTree<TypeTag>(parameters_);
            const auto& gridForConn = simulator_.vanguard().grid();


This object (gridForConn) is unused now.

atgeirr added 2 commits October 8, 2020 15:12

Removed old amg/cpr code that can be replaced.

65817c9

The replacement consists of using the FlexibleSolver code.

Avoid unnecessary extra matrix manipulation.

1c208e9

When using the ParallelOverlappingILU0 it is not necessary to fix the overlap rows.

atgeirr mentioned this pull request Oct 10, 2020

Unify linear solvers #2706

Closed

bska reviewed Oct 10, 2020

View reviewed changes

opm/simulators/linalg/FlowLinearSolverParameters.hpp Outdated Show resolved Hide resolved

bska approved these changes Oct 10, 2020

View reviewed changes

atgeirr added 4 commits October 11, 2020 08:20

Refactor shouldCreateSolver().

0a70d95

Remove unused parts (not just comment out).

00438c4

Refactor setupPropertyTree().

842afd3

Add "amg" option to --linear-solver-configuration.

30a4fe5

bska reviewed Oct 11, 2020

View reviewed changes

opm/simulators/linalg/ISTLSolverEbos.hpp Outdated Show resolved Hide resolved

opm/simulators/linalg/ISTLSolverEbos.hpp Outdated Show resolved Hide resolved

Fix logic of shouldCreateSolver().

3858804

blattms requested changes Oct 12, 2020

View reviewed changes

atgeirr added 3 commits October 12, 2020 16:41

Remove --linear-solver-configuration-file parameter.

f169cf9

This is now handled by passing a string to --linear-solver-configuration that ends with ".json".

Rename parameter --linear-solver-configuration to --linsolver.

316b289

Copyright additions.

dedbe78

Bugfix: do not substr() on too small strings.

ea0abd1

bska reviewed Oct 12, 2020

View reviewed changes

opm/simulators/linalg/setupPropertyTree_impl.hpp Show resolved Hide resolved

blattms approved these changes Oct 13, 2020

View reviewed changes

blattms merged commit 884e02c into OPM:master Oct 13, 2020

bska reviewed Oct 13, 2020

View reviewed changes

atgeirr deleted the unify-linsolve-only branch October 15, 2020 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify linear solvers #2848

Unify linear solvers #2848

atgeirr commented Oct 10, 2020

atgeirr commented Oct 10, 2020

atgeirr commented Oct 10, 2020

ytelses commented Oct 10, 2020

alfbr commented Oct 10, 2020

bska left a comment

atgeirr commented Oct 11, 2020

atgeirr commented Oct 11, 2020 •

edited

Loading

atgeirr commented Oct 11, 2020

bska left a comment

atgeirr commented Oct 11, 2020

blattms left a comment •

edited

Loading

blattms Oct 12, 2020

blattms commented Oct 12, 2020

akva2 commented Oct 12, 2020

blattms commented Oct 12, 2020 •

edited

Loading

atgeirr commented Oct 12, 2020

blattms commented Oct 12, 2020 via email

atgeirr commented Oct 12, 2020

atgeirr commented Oct 12, 2020

atgeirr commented Oct 12, 2020 •

edited

Loading

atgeirr commented Oct 12, 2020

bska Oct 13, 2020

Unify linear solvers #2848

Unify linear solvers #2848

Conversation

atgeirr commented Oct 10, 2020

atgeirr commented Oct 10, 2020

atgeirr commented Oct 10, 2020

ytelses commented Oct 10, 2020

alfbr commented Oct 10, 2020

bska left a comment

Choose a reason for hiding this comment

atgeirr commented Oct 11, 2020

atgeirr commented Oct 11, 2020 • edited Loading

atgeirr commented Oct 11, 2020

bska left a comment

Choose a reason for hiding this comment

atgeirr commented Oct 11, 2020

blattms left a comment • edited Loading

Choose a reason for hiding this comment

blattms Oct 12, 2020

Choose a reason for hiding this comment

blattms commented Oct 12, 2020

akva2 commented Oct 12, 2020

blattms commented Oct 12, 2020 • edited Loading

atgeirr commented Oct 12, 2020

blattms commented Oct 12, 2020 via email

atgeirr commented Oct 12, 2020

atgeirr commented Oct 12, 2020

atgeirr commented Oct 12, 2020 • edited Loading

atgeirr commented Oct 12, 2020

bska Oct 13, 2020

Choose a reason for hiding this comment

atgeirr commented Oct 11, 2020 •

edited

Loading

blattms left a comment •

edited

Loading

blattms commented Oct 12, 2020 •

edited

Loading

atgeirr commented Oct 12, 2020 •

edited

Loading