Handle substeps in LTS error control #5943
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wthrowe
force-pushed
the
lts_error_control
branch
from
48a9273
to
7d2aede
Compare
nilsdeppe
requested changes
Apr 27, 2024
| #include "Time/Time.hpp" | ||
|
|
||
| template <typename T> | ||
| struct StepperErrorEstimate { |
There was a problem hiding this comment.
Could you please add doxygen documentation to this?
src/Time/StepperErrorEstimate.hpp
Outdated
| size_t order{}; | ||
| T error{}; | ||
|
|
||
| void pup(PUP::er& p) { |
wthrowe
force-pushed
the
lts_error_control
branch
from
7d2aede
to
b9a0a4a
Compare
|
Done. Also changed the initializer for |
wthrowe
commented
Apr 30, 2024
| // won't improve the reported error, so make sure to only do it | ||
| // if we actually request a smaller step. | ||
| return std::make_pair(new_step, | ||
| new_step >= previous_step or l_inf_error <= 1.0); |
There was a problem hiding this comment.
Wait, this is backwards.
There was a problem hiding this comment.
Actually, no, it's the right way, but it does need an abs.
There was a problem hiding this comment.
Pushed a fixup for this and updated the test so it would have been caught.
wthrowe
added
in progress
|
And added a small commit fixing a preexisting bug in the test. |
nilsdeppe
reviewed
May 1, 2024
Aside from cleanup, the main effect of this change is that the ErrorControl stepper now always uses the most recent error estimate, where it used to only activate if that estimate was from the most recent evaluation. The only case where that matters is for LTS updating with a multistage method, which used to not work and now uses the error estimates from the previous two steps, as opposed to the current and previous step for single-stage LTS. This matches the behavior for GTS adjustment, so should be acceptable for now.
It's not valid for any TimeStepper, so move logic to the calling code instead of the steppers.
nilsdeppe
approved these changes
May 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Preparation for adding LTS substep methods.
Proposed changes
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments