Infrastructure tweaks to support error-measure based stepping #2841

moxcodes · 2021-02-06T01:30:29Z

Proposed changes

Various small changes to time steppers, step choosers, and step controllers that preserve existing functionality, but allow the possibility of an iterative process to support step rejection and retry.

Code review checklist

The code is documented and the documentation renders correctly. Run
make doc to generate the documentation locally into BUILD_DIR/docs/html.
Then open index.html.
The code follows the stylistic and code quality guidelines listed in the
code review guide.
The PR lists upgrade instructions and is labeled bugfix or
major new feature if appropriate.

moxcodes · 2021-02-06T03:49:47Z

Removed a small part that better fits in a different PR.

moxcodes · 2021-02-08T18:48:44Z

I've dropped the step controller change, and added a fixup for the step chooser documentation.
For the moment, I've prefixed the Separate timestepper history clean-up with drop, as @wthrowe plans to submit a separate PR to perform a similar task in a way that will work better for dense output features; Once that is in, I'll rebase this PR and drop the commit associated with history clean-up from this one.

wthrowe

I'm having trouble quickly getting the history management working in a way compatible with both this and dense output, so let's go with this form so we're not blocking you and I'll change it for dense output later.

wthrowe · 2021-02-09T02:13:31Z

src/Time/TimeSteppers/AdamsBashforthN.hpp

-  ASSERT(history.size() <= order_,
+  ASSERT(history.size() <= order_ + 1,
         "Length of history (" << history.size() << ") "
-         << "should not exceed target order (" << order_ << ")");
+         << "should not exceed target order (" << order_ << ") by more than 1");


What is this change about?

Ah, yeah good catch -- these couple of lines of the hunk should be dropped. It was for an earlier and less elegant attempt at the history cleanup change.

wthrowe · 2021-02-09T02:15:45Z

tests/Unit/Helpers/Time/TimeSteppers/TimeStepperTestUtils.cpp

  for (uint64_t i = 0; i < num_steps; ++i) {
-    take_step(&time, &y, &history, stepper, rhs, step_size);
+    // last parameter: check that we can apply the stepper sometime in the
+    // middle of the evolution without messing anything up.


"apply the stepper twice", maybe? Again later.

moxcodes · 2021-02-09T23:19:30Z

Dropped the part of the hunk that shouldn't have been present, added a fixup for the comment change, and removed the drop: prefix from the commit about the history cleanup.

wthrowe · 2021-02-10T00:55:41Z

Looks good. Squash.

moxcodes · 2021-02-10T01:04:18Z

Squashed.
Cheers!

nilsvu

LGTM, though I'm by no means an expert in time-stepping :)

nilsvu · 2021-02-12T23:49:17Z

src/Time/StepChoosers/StepChooser.hpp

+  /// a strictly smaller step, guided by the requested time step value.
  template <typename Metavariables, typename DbTags>
-  double desired_step(
+  std::pair<double, bool> desired_step(


Would it be reasonable to make the return value here a std::optional<double>, meaning either "do a step with this size" or "don't do a step"? So the question is: Will the "desired step" double be used also when the "acceptance" bool is false? If this is what you mean with "guided by the requested time step value", please clarify, e.g. by rewording to "... the desired step value returned by this function".

The return value is used when the step is rejected to choose the step size when retrying the step; it is assumed that when the step is rejected the returned step size is strictly smaller than the previously tried step. I'll clarify the documentation.

moxcodes · 2021-02-13T00:32:32Z

Squashed in documentation rewording.
Cheers!

nilsvu · 2021-02-15T13:34:25Z

@wthrowe could you take another look at this?

wthrowe · 2021-02-15T21:46:00Z

@moxcodes What do you want to do with respect to #2883? Do you want to drop your history changes here or have me revert them when I'm ready to merge that?

wthrowe · 2021-02-15T21:51:17Z

Also, @nilsleiffischer, please give me some time to respond to things before pinging me. I find it annoying.

moxcodes · 2021-02-15T22:32:32Z

@wthrowe Would it be too much hassle for you to split out the commit that does the history clean-up at the start of the step to a different PR? Then that could be a dependency of both this (with my history clean up change dropped) and your larger PR that makes the self-start changes
The part here that checks that the step can be re-applied depends on the history clean up changes... If I understand the changes in #2883 correctly, there's no change to the cleanup for substep methods, which means that those methods won't be able to be re-applied (because on the conclusion of the full step, the history will be cleaned up and unavailable for retries). I think that's fine with me for now, I can just remove the expectation of substep re-application from this PR as well -- were any of the features that you needed dependent on being able to repeat update_u in substep methods?

wthrowe · 2021-02-16T00:10:31Z

I don't need the update_u changes. I think I just took them to avoid some git conflict. I do use "Add order function to TimeSteppers", but that's simple so I can just cherry-pick it.

You are correct that I haven't done the substep methods, but redoing a substep probably isn't useful anyway. (And redoing a full step is a different challenge.) Substep methods will need to be converted eventually for dense output.

Submitted as #2894.

- necessary to support the error measure update as a `Variables` with a different tag prefix

This is needed for step-unwinding. This adjusts the interface by consistently using the latest volume data in the history instead of using the current `u` value in some steppers

moxcodes · 2021-02-17T23:14:34Z

rebased following merge of #2894, and dropped the history cleanup parts.
Cheers!

moxcodes requested a review from wthrowe February 6, 2021 01:30

moxcodes force-pushed the error_stepping_infra_tweaks branch from c47767a to 6dd8c58 Compare February 6, 2021 03:48

moxcodes force-pushed the error_stepping_infra_tweaks branch 3 times, most recently from dc11cc0 to 1ff9fbd Compare February 8, 2021 18:47

wthrowe requested changes Feb 9, 2021

View reviewed changes

moxcodes force-pushed the error_stepping_infra_tweaks branch from 1ff9fbd to efe1fd8 Compare February 9, 2021 23:17

moxcodes force-pushed the error_stepping_infra_tweaks branch from efe1fd8 to b676e69 Compare February 10, 2021 01:03

wthrowe previously approved these changes Feb 10, 2021

View reviewed changes

nilsvu reviewed Feb 12, 2021

View reviewed changes

moxcodes dismissed wthrowe’s stale review via c3fc226 February 13, 2021 00:31

moxcodes force-pushed the error_stepping_infra_tweaks branch from b676e69 to c3fc226 Compare February 13, 2021 00:31

nilsvu previously approved these changes Feb 13, 2021

View reviewed changes

moxcodes added 4 commits February 17, 2021 13:45

Add error_estimate_order function to TimeSteppers

c3c805c

Permit different type for TimeStepper error measure

7bc72cd

- necessary to support the error measure update as a `Variables` with a different tag prefix

Adjust step operations to permit repeated calls to update_u

2d6ed8c

This is needed for step-unwinding. This adjusts the interface by consistently using the latest volume data in the history instead of using the current `u` value in some steppers

Add step acceptance parameter to StepChoosers

358fb06

moxcodes dismissed nilsvu’s stale review via 358fb06 February 17, 2021 22:46

moxcodes force-pushed the error_stepping_infra_tweaks branch from c3fc226 to 358fb06 Compare February 17, 2021 22:46

wthrowe approved these changes Feb 19, 2021

View reviewed changes

nilsdeppe approved these changes Feb 19, 2021

View reviewed changes

nilsdeppe merged commit 9ccc388 into sxs-collaboration:develop Feb 19, 2021

moxcodes mentioned this pull request Feb 19, 2021

Reorganize time stepper history management #2883

Closed

3 tasks

Infrastructure tweaks to support error-measure based stepping #2841

Infrastructure tweaks to support error-measure based stepping #2841

Uh oh!

Conversation

moxcodes commented Feb 6, 2021

Proposed changes

Code review checklist

Uh oh!

moxcodes commented Feb 6, 2021

Uh oh!

moxcodes commented Feb 8, 2021

Uh oh!

wthrowe left a comment

Choose a reason for hiding this comment

Uh oh!

wthrowe Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

moxcodes Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

wthrowe Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

moxcodes commented Feb 9, 2021

Uh oh!

wthrowe commented Feb 10, 2021

Uh oh!

moxcodes commented Feb 10, 2021

Uh oh!

nilsvu left a comment

Choose a reason for hiding this comment

Uh oh!

nilsvu Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

moxcodes Feb 13, 2021

Choose a reason for hiding this comment

Uh oh!

moxcodes commented Feb 13, 2021

Uh oh!

nilsvu commented Feb 15, 2021

Uh oh!

wthrowe commented Feb 15, 2021

Uh oh!

wthrowe commented Feb 15, 2021

Uh oh!

moxcodes commented Feb 15, 2021

Uh oh!

wthrowe commented Feb 16, 2021

Uh oh!

moxcodes commented Feb 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants