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

Clearly differentiate between participant_dt and dt #245

Merged
merged 3 commits into from
Mar 16, 2023

Conversation

BenjaminRodenberg
Copy link
Member

I think we should be more clear here, because otherwise the double meaning of dt can lead to bugs that are difficult to find. During the preCICE workshop 2023 I found a bug in the code of a user where the statement dt = beginTimeStep() was left out, because dt was fixed to a constant before the timeloop started dt = subcycling_dt. This is totally understandable from my perspective, if you do not use adaptivity. Why should I redefine dt in every time step, if it stays the same?

The goal of the user was to use subcycling. However, this led to the situation that dt was getting smaller than subcycling_dt when getting close to the end of the first window: min(precice_dt, subcycling_dt) returns precice_dt < subcycling_dt and we overwrite dt leading to dt < subcycling_dt for the remainder of the simulation time.

I think this is a situation that we can avoid easily and at the same time we can show clearly that there are basically three different dts:

  1. dt which is the dt that we actually use for the time step
  2. precice_dt which is the maximum allowed dt defined by preCICE
  3. participant_dt (or solver_dt?) which is the dt desired by the solver (fixed or computed), but might conflict with precice_dt

This is still a draft, because we still have the following todos:

  1. Do we want this change?
  2. Apply it consistently to figures, solverdummies etc.

@BenjaminRodenberg BenjaminRodenberg added bug Something isn't working documentation Improvements or additions to documentation labels Feb 20, 2023
@BenjaminRodenberg BenjaminRodenberg self-assigned this Feb 20, 2023
@BenjaminRodenberg BenjaminRodenberg marked this pull request as draft February 20, 2023 10:16
@uekerman
Copy link
Member

Yes, why not. I am only afraid that this will be "high effort – low gain".

@BenjaminRodenberg
Copy link
Member Author

@uekerman Any preference participant_dt or solver_dt? I think both is fine, but would use solver_dt, because it's shorter and we also write double dt; // solver timestep size in the step-by-step guide.

@uekerman
Copy link
Member

Any preference participant_dt or solver_dt?

No, not really. We explain that both are valid options here: https://precice.org/fundamentals-terminology.html#solver-and-participant

@BenjaminRodenberg BenjaminRodenberg marked this pull request as ready for review February 20, 2023 16:49
@BenjaminRodenberg
Copy link
Member Author

@uekerman I hope I did not oversee any necessary changes.

One thing I just stumbled over: precice_dt or dt_precice? Same for solver_dt. The text in https://precice.org/couple-your-code-timestep-sizes.html sometimes mentions dt_precice. Thinking about the mathematical notation $\Delta t_\text{preCICE}$, dt_precice would probably make more sense. Same for $\delta t_\mathcal{D}$ in https://precice.org/couple-your-code-waveform.html.

Note: Our tutorials currently use precice_dt or my_dt (e.g. here)

Pragmatic suggestion: Let's keep precice_dt, because I think this is really only about consistency (and means a lot of work) and it does not really influence readability.

@fsimonis
Copy link
Member

Just for completeness: Once we rename the SolverInterface to Participant precice/precice#1458, then using participant_dt for the solver would be highly confusing.

Copy link
Member

@uekerman uekerman left a comment

Choose a reason for hiding this comment

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

Looks good

@uekerman uekerman merged commit bc72a03 into master Mar 16, 2023
@uekerman uekerman deleted the differentiate-dt-and-participant-dt branch March 16, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants