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 update of Dirichlet BC and RHS #383

Merged

Conversation

BenjaminRodenberg
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg commented Oct 10, 2023

Fixes bug, if time step changes inside window. See precice/fenics-adapter#20.

For the purpose of documentation, if will add some comments in the diff of this PR.

With this bugfix the waveform version of the tutorial provided via #281 also works for non-matching subcycling, like described in precice/fenics-adapter#20.

Copy link
Member Author

@BenjaminRodenberg BenjaminRodenberg left a comment

Choose a reason for hiding this comment

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

Explaining the pitfall that has lead to this bug in the comments below.

Comment on lines -242 to -244
# Update Dirichlet BC
u_D.t = t + float(dt)
f.t = t + float(dt)
Copy link
Member Author

Choose a reason for hiding this comment

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

This dt is actually the dt from the timestep that was just completed, not the timestep that one is going to compute next. But the u_D and f here is used for the future timestep. Meaning: If dt changes from one timestep to the next, u_D and f will use the dt corresponding to the last time step and not corresponding to the next.

Comment on lines 190 to 198
precice_dt = precice.get_max_time_step_size()
dt.assign(np.min([fenics_dt, precice_dt]))

# Dirichlet BC and RHS need to point to end of current timestep
u_D.t = t + float(dt)
f.t = t + float(dt)

# Coupling BC needs to point to end of current timestep
read_data = precice.read_data(dt)
Copy link
Member Author

Choose a reason for hiding this comment

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

As a general advice, one should set all boundary conditions ("real" boundary conditions, RHS and coupling boundary conditions) before calling solve and before calling advance.

@BenjaminRodenberg
Copy link
Member Author

@NiklasKotarsky and @uekerman might be interesting for you. I think this also nicely illustrates how get_max_time_step_size() improves the API, because before it was much harder to not get confused with the dt returned by preCICE.

@BenjaminRodenberg BenjaminRodenberg merged commit ddbc09f into precice:develop Oct 10, 2023
2 checks passed
@BenjaminRodenberg BenjaminRodenberg deleted the fix-dirichlet-and-rhs branch October 10, 2023 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant