[CoSim] Fix floating-point tolerance in KeepAdvancingSolutionLoop (CoSimAnalysis)#14554
[CoSim] Fix floating-point tolerance in KeepAdvancingSolutionLoop (CoSimAnalysis)#14554juancamarotti wants to merge 5 commits into
Conversation
|
Rather than relying on shady floating-point comparisons, I recommend sending an extra signal from each solver that they finished/terminated. |
That is done for the external solver side, and it's not enough. If we don´t do this, kratos tries to advance to the next time step even when the external solver already disconnected. |
matekelemen
left a comment
There was a problem hiding this comment.
Please wait.
I don't understand. How can the external solver disconnect (without an exception) if CoSim did not tell it to do so. Alternatively, why does Kratos attempt to continue to the next time step if the external solver signaled it to stop?
The two termination mechanisms are independent. The external solver decides whether to terminate based on the control signals exchanged through the solver wrapper, whereas the Kratos CoSimulation framework decides whether to execute another global time step exclusively through KeepAdvancingSolutionLoop(). Therefore, the external solver can correctly identify that the final time has been reached and finalize, while Kratos may still decide to advance if its own end-time check evaluates to True. In the example I run, the current time was 1.999999999999999 instead of exactly 2.0 due to floating-point round-off. Since KeepAdvancingSolutionLoop() relied on an exact comparison, Kratos incorrectly started one additional time step, while the external solver had already reached its termination criterion (because a tolerance is also used in the solver wrapper to deal with this). @sunethwarna @philbucher could you add something more here? |
|
@matekelemen I'll try to explain the issue more clearly. Kratos and OpenFOAM have independent time-marching algorithms. OpenFOAM uses its own On the other hand, due to floating-point round-off, Kratos may reach a time such as Does that make the issue clearer? |
|
|
||
| ## Stepping and time settings | ||
| self.end_time = self.cosim_settings["problem_data"]["end_time"].GetDouble() | ||
| self.time_tolerance = self.cosim_settings["problem_data"]["time_tolerance"].GetDouble() |
There was a problem hiding this comment.
I wouldn't expose this but hardcode it (this is what we normally do for floating point comparisons of this sort).
There was a problem hiding this comment.
Literally i asked for this, you never know how small is going to be the time step considered in the simulation, imagine a coupling of a explicit simulation where 1e-12 could be significative...
There was a problem hiding this comment.
Well, if you reach the point in which the time step is of the order of the floating point comparison, I'd say that your problem is not the comparison but a different thing... (even though you're explicit) In any case I don't want to block it because of this. Go ahead if you really need this. I just want to keep the I/O as simple as possible.
|
@juancamarotti I'm familiar with the concept of floating point rounding errors. My issue is not that, but problem of which solver is in control. Forget about time. In a scenario with two domains-specific solvers In an implicit coupling scenario, this has to be the coupling solver Your problem is that I hope you understand that this fix does not address the underlying problem at all. I can configure In my opinion, the correct fix is making Alternatively, if you really really want to, you could implement a way for |
|
@matekelemen I'm sorry but I don't agree with you. The solvers can be notified through the solver wrapper when to disconnect (and this already happens with the current openfoam wrapper), but not when to terminate. The termination must happen at the same time step for all of them. @sunethwarna also agrees on this. You can discuss it in person with him. |
I don't get what the difference between disconnecting and terminating is from the perspective of CoSim. It makes very little sense to me that a solver keeps on doing time steps after it has disconnected from CoSim. Once a solver gets an order to disconnect, it should clean up, do its post-processing or whatever, and terminate. If you rely on all solvers having a predetermined time at which they terminate,
|
|
@philbucher what's your opinion on this? |
If you want to implement a "better" solution, feel free to do it. This is all the time I will spend on this. |
|
@matekelemen could you please remove your block? |
I will not. If you want a quick-and-dirty solution, go increase the end time of your OpenFOAM solver. If you want this change, open a PR for the core |
Description
This PR fixes an issue in the
CoSimulationAnalysistime loop where an additional time step could be executed due to floating-point round-off errors when reaching the end of the simulation.Previously, the default implementation of
KeepAdvancingSolutionLoop()could continue the simulation even when the current time was numerically equal to the specifiedend_time(e.g.1.999999999999999vs.2.0). As a consequence, the coupled analysis advanced one extra time step, leading to synchronization issues with external solvers that had already finalized.This PR overrides
KeepAdvancingSolutionLoop()to stop the simulation once the current time reaches the prescribedend_timewithin a specified tolerance.Motivation
This issue was observed during partitioned Fluid–Structure Interaction simulations with OpenFOAM through CoSimIO. While the external solver correctly finalized at the last time step, the CoSimulation loop advanced once more due to floating-point inaccuracies, causing the coupled simulation to deadlock while waiting for data from an already finalized solver.
Changes
KeepAdvancingSolutionLoop()inCoSimulationAnalysis.end_timeto determine whether the solution loop should continue.Result
The coupled simulation now terminates consistently at the prescribed end time, avoiding unnecessary extra time steps and ensuring proper synchronization with external solvers.