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

Include rejection of time step if tolerance test fails #5317

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

erikhide
Copy link

@erikhide erikhide commented Apr 26, 2024

Most of the work published on adaptive time stepping includes a test for deciding whether or not to reject the most recent time step. This is usually done by taking the relative change or some error estimate and comparing this to one or more threshold value(s). If the change/error is too large, the time step is rejected, and we try again with a shorter time step. In other words, we discard the computations and move back to the previous point in time. Currently, however, this is not done in OPM, and my belief (still only a hypothesis) is that this has contributed to the lack of robustness in the "pure" PID controller implementation.

Note that the OPM Flow Reference Manual includes a footnote reference to "Algebraic Flux Correction III. Incompressible Flow Problems. Uni Dortmund, Turek and Kuzmin, January, 2006 (DOI: 10.1007/3-540-27206-2_8)" in which the method clearly states that a failure to satisfy the tolerance test should result in a rejection of the time step.

I have implemented such a rejection of time steps now. It seems to be working as it should, but please let me know if I should change something.

erikhide and others added 4 commits April 26, 2024 13:11
…lation now rejects the time step and tries again with a shorter time step
… this is now done in AdaptiveTimeStepping.hpp, including a rejection of the time step
@bska
Copy link
Member

bska commented Apr 26, 2024

Much appreciated! Just for clarity: Is this work intended as a replacement for, or perhaps a continuation of, #4855 and/or #4856? Or maybe it's entirely independent of those two existing PRs?

@erikhide
Copy link
Author

This is meant to solve an independent problem in the adaptive time stepping.

The pull request does in fact include the changes done in #4855, but the new changes related to rejection of time steps are independent of that. They are also completely independent of #4856.

@bska
Copy link
Member

bska commented Apr 26, 2024

This is meant to solve an independent problem in the adaptive time stepping.

Okay, understood.

The pull request does in fact include the changes done in #4855, but the new changes related to rejection of time steps are independent of that.

Very good. Then I think we should maybe change the state of this PR to draft/work-in-progress pending #4855 so we don't inadvertently merge this in conflict with that work. Would that be okay with you?

@erikhide
Copy link
Author

Very good. Then I think we should maybe change the state of this PR to draft/work-in-progress pending #4855 so we don't inadvertently merge this in conflict with that work. Would that be okay with you?

Sure! That's probably a good solution.

@bska bska marked this pull request as draft April 26, 2024 11:55
@bska
Copy link
Member

bska commented Apr 26, 2024

Very good. Then I think we should maybe change the state of this PR to draft/work-in-progress pending #4855 so we don't inadvertently merge this in conflict with that work. Would that be okay with you?

Sure! That's probably a good solution.

Cool–I've marked it as such now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants