Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix time accumulation and max time #1933

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

fsimonis
Copy link
Member

@fsimonis fsimonis commented Jan 19, 2024

Main changes of this PR

This PR

  • replaces the time window start with a Kahan aggregator
  • constrains the max dt by max-time

Motivation and additional information

time-window-size = 0.01 and max-time = 5 leads to 501 time windows due to rounding errors.
This should solve the problem.

Author's checklist

  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I added a changelog file with make changelog if there are user-observable changes since the last release.
  • I added a test to cover the proposed changes in our test suite.
  • For breaking changes: I documented the changes in the appropriate porting guide.
  • I sticked to C++17 features.
  • I sticked to CMake version 3.16.3.
  • I squashed / am about to squash all commits that should be seen as one.

@fsimonis fsimonis added the bug preCICE does not behave the way we want and we should look into it (and fix it if possible) label Jan 19, 2024
@fsimonis fsimonis added this to the Version 3.0.0 milestone Jan 19, 2024
@fsimonis fsimonis merged commit f84d871 into precice:develop Jan 19, 2024
19 checks passed
@fsimonis fsimonis deleted the fix-time-stepping branch January 19, 2024 10:27
Comment on lines +497 to +498
double leftover = _maxTime - _time;
return std::min(maxDt, leftover);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to forward here an arbitrary small dt? I would say that the current behavior is numerically still more robust. Not sure that this PR is the right place to add it into the library.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous state was flatout wrong.
time-window-size = 0.3 and max-time = 1 resulted in a timestep from 0.9 to 1.2.

Now the final time-window-size is correctly synchronized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With participant-first methods and adaptive time-stepping (e.g. in OpenFOAM) this can still lead to issues. In the adapter, we set the end time to infinity such that preCICE steers it. Hence, the solver is not aware of the maxTime limit and this return value can lead o arbitrary small dt causing numerical issues then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that this only applies to hasTimeWindowSize(). What happens to the second participant in a participant-first time-stepping. Do we then perform different time-step length for different participants?

Copy link
Member Author

@fsimonis fsimonis Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For participant-first there are two cases:

  • first doesn't have a time window size as it is dictating it. The getNextTimeStepMaxSize() is maxTime if provided and inf otherwise.
    This was correctly implemented.
  • second has a dynamic time window size which it received from first, which is correctly limited by max-time. So in theory this cannot surpass max-time.

src/cplscheme/BaseCouplingScheme.cpp Show resolved Hide resolved
src/cplscheme/BaseCouplingScheme.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug preCICE does not behave the way we want and we should look into it (and fix it if possible)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants