Discussion: role of Simulator classes.

[atgeirr]

I propose the following responsibilities for the Simulator classes, in particular SimulatorFullyImplicitBlackoil:

• Handle the timesteps. This means that the loop over all (report) timesteps should be in this class, and also that this class implements (or uses implementations of) sub-timestepping strategies that take into account convergence, number of nonlinear iterations etc.
• Handle output. I think timesteps and output must be handled at the same level. The lower level FullyImplicitBlackoilSolver class cannot know if what it is computing will be used (trying to take too long a time step), or if output is supposed to be written (if it is in a ministep and the user has chosen not to output intermediate steps between report steps).
• Handle changes to the well configuration. This includes not only obtaining the well controls etc. but also initialising and propagating well states correctly when such things change.

I propose that the following responsibility is removed from the class:

• Handle control switching due to constraints being broken. This must be handled at a lower level so that it can change from one Newton iteration to the next, which seems to be necessary for robustness (this is part of the work being done with the well formulation now).

This requires the following changes to interfaces between levels (sim_fibo_ad, Simulator class and Solver class):

• The Solver class must report the number of nonlinear iterations, max changes to saturation and any other data required by the stepsize logic that will reside in the Simulator class.
• The Simulator class must have a way to get the report timesteps. I propose using the SimulatorTimer class.
• The Simulator class must have a way to (for each new timestep) to create a new well structure. We may want to pass in the parser object for this, or (more general) an object that can construct the required structure on demand. We may also generate something like an array of WellsHandler objects, one for each timestep, and pass that to the Simulator class, but I think this will be too wasteful in terms of memory etc.

Please tell me what you think, before we start working on this.

(Update: forgot to mention that this is related to issues #103 and #106)

[bska]

The Wi-Fi connection here is flaky so I must be brief. I agree with most/everything you say, @atgeirr, except I think we need to be a bit more specific about the following

The Solver class must report the number of nonlinear iterations, max changes to saturation and any other data required by the stepsize logic that will reside in the Simulator class.

In principle, step size logic could need a large number of disparate data sources (e.g., the complete state object of the past several time steps). This all depends on the controller. Requesting that the solver report any data required by step size logic is, in my opinion, too onerous. That, I think, requires some kind of policy parameter to translate the solver's view of the state into the controller's input data. Or maybe you have something less ambitious in mind?

[atgeirr]

@bska: Yes, I did not intend to make a universal solution now, but something less ambitious. I imagine putting (initially) a boolean (converged) and an integer (number of iterations) in a struct and having the solver return it. Then replace by something more comprehensive only as it becomes necessary.

[bska]

Okay, then. As a first step, I think that's an acceptable proposal.

[andlaus]

I agree with most stuff as well, but I've got a suggestion: The tasks for the simulator class sounds to be very generic (writing output, handling timesteps), so I think there is not much point for having a separate class for each model. Instead, we could have one templated class doing this crunch work (e.g. Simulator<Solver>) which just calls the solver to do anything model related...

anyway, its a good idea to simplify everything which needs to be done in main(). N.B.: in eWoms the main file just looks like this:

#include "config.h"
#include <ewoms/common/start.hh>
#include "myproblem.hh"

int main(int argc, char **argv)
{
    typedef TTAG(MyProblem) TypeTag;
    return Ewoms::start<TypeTag>(argc, argv);
}

and I think that's where the other modules should be heading as well...

[atgeirr]

You have my full agreement on both suggestions, @andlaus, but one caveat. While we like templates, many people are put off by them. Anyway, I'd like us to get this done for the fully implicit simulator, then we can generalize after that.

[andlaus]

ok, quite likely, this can equally well be achieved using virtual functions (since that code is in no way performance critical, I also won't object doing it this way)...

[GitPaean]

This means that the loop over all (report) timesteps should be in this class ......

The loop will be based on the report steps or simulation steps (without considering the sub-timestepping due to the convergence control), or they are basically the same in your concept?

I think they will be different, even in your description. While which one the class will be base on?

[joakim-hove]

With all disclaimers applied - my reaction is that this:

Handle the timesteps. This means that the loop over all (report) timesteps should be in this class, and also that this class implements (or uses implementations of) sub-timestepping strategies that take into account convergence, number of nonlinear iterations etc.

is too much - in my opinion the Simulator class should just step forward a number of seconds, the looping over report steps should be done externally:

for (report_step = 0; ...) {
     size_t seconds = timeMap.getDelta( report_step);
     simulator.run( seconds );
}

This will give much more flexibility in manipulating the simulator state; one could e.g. consider changing simulator instance runtime?

[andlaus]

@joakim-hove that's basically how it's currently done in sim_fibo_ad.cpp (modulo writing output ;))

[joakim-hove]

@joakim-hove that's basically how it's currently done in sim_fibo_ad.cpp (modulo writing output ;))

Yes - but in the current sim_fibo_ad.cpp a new simulator instance is instantiated for every time step - that should not be necessary.

[andlaus]

Yes - but in the current sim_fibo_ad.cpp a new simulator instance is instantiated for every time step - that should not be necessary.

agreed. but, that's just an implementation detail IMO. as far as I understood this issue here, it is rather about how to best split the work between the simulator, solver and main function...

[atgeirr]

@joakim-hove A comment on your note from a few days ago: stepping forward a set amount of time is indeed the sole purpose of a specific class, but it is the solver class, not the simulator class. Which is why I see the simulator class as the natural one-level-higher place to do timestep control.

The solver class has a method step() taking a const double dt just as you suggest, the simulator class has a method run(). I intend that it should gain a private method stepOneReportStep() (or similar) as well, to separate the basic timestepping logic from the logic handling convergence control and substeps.

[GitPaean]

In the current implementation, we recreate or initialize the simulator class for each time step or report step. For the Norne case, the initialization of the simulator takes more than half minute each time. Each newton iteration takes something like 4 seconds. I feel that can be a problem, since it can mean we will need more than one and a quarter hour extra for a complete Norne simulation to do the recreation of the simulator class.