Kratos: Proposal for breaking main loop (new method to be overriden in AnalysisStage)

[maceligueta]

Recently, at the @KratosMultiphysics/dem team, we found ourselves with the need of stopping a case due to diverse causes (statistic results converge, external stopping signal, ...).
We found this solution satisfactory:
https://github.com/KratosMultiphysics/Kratos/blob/6d198969c422f63e474435195a35dcc3043fa3c2/applications/DEM_application/python_scripts/main_script.py#L457
The BreakSolutionStepsLoop method can be customized to return true and stop the computation in a clean way (finishing the loop and going to Finalize).
I think it is a useful feature. If it looks good to everyone we can add it to the AnalysisStage.
I tag this as discussion because it's just a proposal.
Referencing @KratosMultiphysics/technical-committee and @philbucher (as traditional developer of AnalysisStage).

[pooyan-dadvand]

Just to say that clean breaking is also an issue for other applications

[jcotela]

I believe that what you need to do is to have RunSolutionLoop check for your break condition. I think it would be helpful if you could try it on your main DEM stage as a first step/proof of concept so that we can all see what exactly is needed. Then we can discuss if and how to move the relevant functions to the base class as part of the PR.

[loumalouomega]

@maceligueta I am currently developing an adaptative analysis, for that I started with the implementation of factories for the solving strategies: https://github.com/KratosMultiphysics/Kratos/pull/3185

The idea is to do it in such general way that we can integrate utilities and processes to check that the problem converges, the geometries is not inverted and so on

[loumalouomega]

Think that for an implicit solution the problem can broke during non linear iterations

[philbucher]

@maceligueta just to understand you correctly, you want to do the following right?
~py
def RunSolutionLoop(self):
"""This function executes the solution loop of the AnalysisStage
It can be overridden by derived classes
"""
while self.time < self.end_time:
self.time = self._GetSolver().AdvanceInTime(self.time)
self.InitializeSolutionStep()
self._GetSolver().Predict()
self._GetSolver().SolveSolutionStep()
self.FinalizeSolutionStep()
self.OutputSolutionStep()
self.BreakSolutionLoop() # this is the new function, which would be empty in the baseclass
~

[loumalouomega]

@philbucher I discussed with @RiccardoRossi something like this

~~~
def RunSolutionLoop(self):
"""This function executes the solution loop of the AnalysisStage
It can be overridden by derived classes
"""
while self.time < self.end_time:
is_converged = False
while(not is_converged):
self.time = self._GetSolver().AdvanceInTime(self.time)
self.InitializeSolutionStep()
self._GetSolver().Predict()

            is_converged = self._GetSolver().SolveSolutionStep()
            if(not is_converged):
                self.time = self._GetSolver().ResetTimeStep()
                self.GetSolver().ReduceTimeStep()

        self.FinalizeSolutionStep()
        self.OutputSolutionStep()

~~~

[philbucher]

@loumalouomega your proposal looks reasonable, however I am not sure if this can go mainstream, i.e. I am not sure if the adaptivity should be in the baseclass.

1. I think in some cases it is acceptable to have a non-converged solution in the beginning of the simulation. E.g. in FSI we useally accept non-converged solutions in the first steps, it stabilizes itself afterwards
   I.e. I don't want to reduce the timestep there
2. I can imagine that this can also be done in different ways, i.e. other physics might do it differently, then this would be a limitation

BTW are you sure you should be calling AdvanceInTime several times per timestep?

Overall this seems like a different discussion to me

#

My proposal (for both the issues of @loumalouomega and @maceligueta and also others (ping @sunethwarna for problems that are backwards in time) ):
Separate the content inside the time loop:

~~~py
"""in AnalysisStage.py:"""
def RunSolutionLoop(self):
while self.time < self.end_time:
self.SolveSolutionStep() # name up for discussion, just a placeholder

def SolveSolutionStep(self):
self.time = self._GetSolver().AdvanceInTime(self.time)
self.InitializeSolutionStep()
self._GetSolver().Predict()
self._GetSolver().SolveSolutionStep()
self.FinalizeSolutionStep()
self.OutputSolutionStep()
# self.BreakSolutionLoop() # for discussion

"""in AnalysisStageMiguel.py:""" # => needed if BreakSolutionLoop is not in the BaseClass
class AnalysisStageMiguel(AnalysisStage):
def SolveSolutionStep(self):
super(AnalysisStageMiguel, self).SolveSolutionStep()
self.BreakSolutionLoop()

"""in AnalysisStageVicenteAdaptive.py:""" #
class AnalysisStageVicenteAdaptive(AnalysisStage):
def SolveSolutionStep(self):
is_converged = False
while(not is_converged):
self.time = self._GetSolver().AdvanceInTime(self.time)
self.InitializeSolutionStep()
self._GetSolver().Predict()

        is_converged = self._GetSolver().SolveSolutionStep()
        if(not is_converged):
            self.time = self._GetSolver().ResetTimeStep()
            self.GetSolver().ReduceTimeStep()

    self.FinalizeSolutionStep()
    self.OutputSolutionStep()

[loumalouomega]

What do you think @RiccardoRossi ?

[RiccardoRossi]

i have read quickly through the different proposals and i think many of them make sense.

the summary of @philbucher looks quite clean to me, nevertheless the argument that a functionality similar to BreakSolutionLoop enters in the base class also has merit. Having said this, reimplementing as in the latest proposal is as clean as it gets...

[jcotela]

:+1: for @philbucher's proposal, just with a minor comment:

"""in AnalysisStageMiguel.py:""" # => needed if BreakSolutionLoop is not in the BaseClass
class AnalysisStageMiguel(AnalysisStage):
    def SolveSolutionStep(self):
        super(AnalysisStageMiguel, self).SolveSolutionStep()
        if self.BreakSolutionLoop():
            break

I am not opposed to adding support for this in the base class, as @RiccardoRossi says, if it is needed.

[philbucher]

I was just discussing with @jcotela and @sunethwarna and we were thinking abt a different approach:
~~~py
"""in AnalysisStage.py:"""
def RunSolutionLoop(self):
"""This function executes the solution loop of the AnalysisStage
It can be overridden by derived classes
"""
while self.SolutionIsOngoing(): # bad name, I know
self.time = self._GetSolver().AdvanceInTime(self.time)
self.InitializeSolutionStep()
self._GetSolver().Predict()
self._GetSolver().SolveSolutionStep()
self.FinalizeSolutionStep()
self.OutputSolutionStep()

def SolutionIsOngoing(self):
return self.time < self.end_time

"""in AnalysisStageMiguel.py:"""
class AnalysisStageMiguel(AnalysisStage):
def SolutionIsOngoing(self):
# perform checks if solution is still ongoing ...
return True / False
~~~

this does not need any breaking mechanism and is also suitable for other problems (i.e. adjoint => backward in time)

[jcotela]

@philbucher I think that if should be a while, I edited it in the github issue.

[maceligueta]

Note that if we use break, the word breakmust be at the same level as the whileit is breaking (can't be encapsulated in function).
The approach
while KeepGoing():
instead of
while self.time < self.end_time:
sounds good to me as well, but I find it less intuitive (takes longer to read for a newbie).

[maceligueta]

Thinking it better, the approach while KeepAdvancingSolutionLoop(): would be more general, and would be valid for advancing steps or other variables used for controlling the loop (load increments?).

[philbucher]

I think we are going in a good direction

Since this affects everyone I propose to leave this open until next week and then see what the opinions of others are by then

[philbucher]

Related to this, I saw in #3335

@GuillermoCasas

Can we add a method to the AnalysisStage which includes all the other methods in the main loop? Like SolveFullStep...
@RiccardoRossi says that it's very important to force everyone to use all those methods that are now explosed.

@GuillermoCasas would such a function still be needed if the proposal of @maceligueta is there?
no objections, just asking

[maceligueta]

I think that @GuillermoCasas asked for that method in order to save lines of code. Because when you are coding a simple coupling stage, you can have

while time < final_time:
    app1_solution.SolveFullStep()
    app2_solution.SolveFullStep()

instead of having

while time < final_time:
    self.app1_solution.InitializeSolutionStep()
    self.app1_solution._GetSolver().Predict()
    self.app1_solution._GetSolver().SolveSolutionStep()
    self.app1_solution.FinalizeSolutionStep()
    self.app1_solution.OutputSolutionStep()

    self.app2_solution.InitializeSolutionStep()
    self.app2_solution._GetSolver().Predict()
    self.app2_solution._GetSolver().SolveSolutionStep()
    self.app2_solution.FinalizeSolutionStep()
    self.app2_solution.OutputSolutionStep()

Well, this was a simplification, but I agree that in some cases a method including all the methods in the while might be useful.

[philbucher]

To me it makes sense adding such a method
But AdvanceInTime should be included imo

[maceligueta]

Sure! It was a simplified version, just to understand the need of it.
However, as I wrote in #3335, @RiccardoRossi is quite against this.

[GuillermoCasas]

@philbucher: Yes, @maceligueta is right, I just wanted to save lines and also inherit any changes automatically. I am still not sure I understand why @RiccardoRossi is against including that function. Can anyone enlighten me on that one?

[philbucher]

Hm I guess that he can that only answer himself

what I suspect (and also did not know previously) is that SolveSolutionStep is the only function that is allowed to be called multiple times per time-step. (e.g. for coupling)
Am I right @RiccardoRossi ?

I can imagine that if we provide the SolveFullStep to users they might end up using this one bcs it is more convenient and hence make a mistake
=> hence for coupling I would strongly suggest to use the 6 functions separately, even if it is more code
If you do implicit coupling (solving several times per time-step) then there is no other way of calling them separately!

~py
while time < final_time:
self.app1_solution.InitializeSolutionStep()
self.app2_solution.InitializeSolutionStep()
self.app1_solution._GetSolver().Predict()
self.app2_solution._GetSolver().Predict()
while not convergence_achieved:
self.app1_solution._GetSolver().SolveSolutionStep()
self.app2_solution._GetSolver().SolveSolutionStep()
self.app1_solution.FinalizeSolutionStep()
self.app2_solution.FinalizeSolutionStep()
self.app1_solution.OutputSolutionStep()
self.app2_solution.OutputSolutionStep()
~

take a look at the FSI of @rubenzorrilla for a more complex example:https://github.com/KratosMultiphysics/Kratos/blob/master/applications/FSIapplication/python_scripts/partitioned_fsi_base_solver.py

other scenarios users might want to do and which is already possible (without overriding RunMainSolutionLoop):
~py
while time < final_time:
# do some crazy stuff before the solution
# => this can already be done by overriding InitializeSolutionStep of the AnalysisStage
# or e.g. overriding ModifyInitialProperties/Geometry
app1_solution.SolveFullStep()
# do some fancy postprocessing or other things
# also this could be done by overriding OutputSolutionStep
~

[GuillermoCasas]

@philbucher: Probably, yes. However, I think the function is going to be created by the derived classes very often anyway, since successive derivations of the class will very likely want to inherit the same calls (and in the same order) as the parent. The existence of the class in the base invites coders to consider this possibility from the start, leading to uniform naming throughout kratos. The argument about it being only callable once per time step is a good one, though. However, the scenario where developers would be calling such a general function mindlessly looks unlikely to me.

[philbucher]

@GuillermoCasas I see

I guess this will have to be discussed by the @KratosMultiphysics/technical-committee
@maceligueta can you add it when you discuss the KeepAdvancingSolutionLoop?

[maceligueta]

Sure! I will comment this.

[RiccardoRossi]

@GuillermoCasas the argument that @philbucher presented are correct, in particular we need to ensure that the FinalizeSolutionStep is called strictly once.

I am personally very much against defining a SolveFullStep function (it was there and we removed it) since by NOT defining you are obliging people implementing a solver to actually implement the split functions. If you were to have the SolveFullStep function than one may implement only that without implementing the other subdivided functions.

Note on the other hand that one is not allowed (at least by design) to increase the public API of an Analysis. (of course in python you can actually do ... but it is not the intended use, in the sense that it breaks the idea that the base class defines the aPI).

This means that if you add a function SolveFullStep
than you SHOULD name it _SolveFullStep with a _ at the beginning to indicate that it is protected but not public.

as a final comment, one simple idea to reduce the verbosity would be to have a "SequentialCoupledSolver" which incorporates the two solvers and just wraps up the calls, so that externally you can minimise the interface. Just take a look at

https://github.com/KratosMultiphysics/Kratos/wiki/Example:-A-simple-Fluid---Thermal-coupled-solver

[pooyan-dadvand]

Forwarding to @KratosMultiphysics/design-committee

[philbucher]

The @KratosMultiphysics/design-committee discussed this and we support the implementation of KeepAdvancingSolutionLoop

Minor: We are not super-happy with the name of the method but we could not come up with a better one so its ok for us.

Whoever implements it should document clearly what the function does