Add LTS support to Adams-Moulton time steppers #6043

wthrowe · 2024-05-29T21:35:57Z

Proposed changes

Upgrade instructions

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
new feature if appropriate.

Further comments

nilsdeppe

LGTM, a few clarifying suggestions. Please squash and rebase immediately :)

nilsdeppe · 2024-05-30T02:12:09Z

src/Time/TimeSteppers/AdamsLts.cpp

 namespace TimeSteppers::adams_lts {
+Time exact_substep_time(const TimeStepId& id) {
+  ASSERT(id.substep() == 0 or id.substep() == 1,
+         "Implemented Adams-based methods have no more than one substep.");


Maybe "The implemented... If you implemented a new method, you should..."? So then if someone adds one they have some guidance on what to do?

They'd have to code in how big the extra substeps are, I guess? Assuming the value for substep 1 still agreed with AM? I'm not sure what you're asking for. I don't think there's anything subtle going on here. This is just the calculation of id.substep_time() but as a Time instead of a double.

Oh, I see. I think what would've helped is the phrasing "The currently implemented.... If you've implemented one that has more substeps then you must generalize this code."

Decided to replace it with a check that the result is correct, instead of a whitelist on the input.

nilsdeppe · 2024-05-30T02:12:56Z

src/Time/TimeSteppers/AdamsLts.cpp

+                                 const AdamsScheme& small_step_scheme) {
+  ASSERT(
+      small_step_scheme == local_scheme or small_step_scheme == remote_scheme,
+      "FIXME just deduce?");


Did you mean to leave these FIXMEs?

Rebase fail. Removed altogether instead of adding in one commit and deleting later.

src/Time/TimeSteppers/AdamsLts.cpp

nilsdeppe · 2024-05-30T02:16:27Z

src/Time/TimeSteppers/AdamsLts.cpp

+                         (small_step_scheme.type == SchemeType::Implicit ? 2
+                                                                         : 1)];
+    ASSERT(not time_less(current_small_step, start_time),
+           "Missed step start iterating over small steps.");


Could you expand on this message a bit? As a user I'm not sure what I should do if I hit this. Maybe also print out current_small_step and start_time so if it does show up at least someone can give us those values for their simulation?

nilsdeppe · 2024-05-30T02:17:46Z

src/Time/TimeSteppers/AdamsLts.cpp

+      small_step_end = current_small_step;
+    } else {
+      ERROR(
+          "Multiple-small-step dense output is not supported.  Split into an "


Sorry, I'm also not entirely sure what I should change if I hit this error :(

nilsdeppe · 2024-05-30T02:18:16Z

src/Time/TimeSteppers/AdamsLts.cpp

+  }
+
+  // Combine duplicate entries
+  ASSERT(not step_coefficients.empty(), "Generated no coefficients");


maybe add "... This an algorithmic bug. Please file an issue."?

nilsdeppe · 2024-05-30T15:39:26Z

src/Time/TimeSteppers/AdamsLts.cpp

+  // The sides are stepping at the same rate, so no LTS is happening
+  // at this boundary.
+  OrderVector<Time> control_times(control_ids.size());
+  std::transform(control_ids.begin(), control_ids.end(), control_times.begin(),


[optional] alg::transform?

nilsdeppe · 2024-05-30T15:39:46Z

src/Time/TimeSteppers/AdamsLts.cpp

+  }
+
+  OrderVector<double> control_times_fp(control_times.size());
+  std::transform(control_times.begin(), control_times.end(),


[optional] alg::transform?

wthrowe · 2024-05-31T21:04:47Z

src/Time/TimeSteppers/Factory.hpp

 /// Typelist of available LtsTimeSteppers
-using lts_time_steppers = tmpl::list<AdamsBashforth>;
+using lts_time_steppers =
+    tmpl::list<AdamsBashforth, AdamsMoultonPc<false>, AdamsMoultonPc<true>>;


Note to myself: Make the GH executables only accept the non-deadlocking time steppers in LTS mode.

Added monotonic_lts_time_steppers in a new commit.

knelli2

Couple clang-tidy things and also some timeouts in the AB tests, but other than that the code looks ok. But to be honest, I didn't really understand it all that much (nor am I going to try to). I will say that this PR could have benefited from being split up into a few smaller PRs.

wthrowe

Added two more fixups. One of the clang-tidy errors was for a feature our compilers don't support yet, so I disabled the check for now.

wthrowe · 2024-06-11T19:58:10Z

src/Time/TimeSteppers/AdamsLts.cpp

 namespace TimeSteppers::adams_lts {
+Time exact_substep_time(const TimeStepId& id) {
+  ASSERT(id.substep() == 0 or id.substep() == 1,
+         "Implemented Adams-based methods have no more than one substep.");


Decided to replace it with a check that the result is correct, instead of a whitelist on the input.

nilsdeppe

fixups LGTM. Please rebase and squash!

Most of the old version assumed properties of Adams-Bashforth, even when it didn't intend to. These new tests should work for other methods as well.

The monotonic Adams-Moulton method requires a similar amount of history on the local side as the LTS methods generally require on the remote side.

As per clang-tidy.

The others LTS steppers will deadlock.

nilsdeppe requested changes May 30, 2024

View reviewed changes

wthrowe commented May 31, 2024

View reviewed changes

wthrowe force-pushed the predictor-corrector_lts branch from e00b89b to 9be3826 Compare June 11, 2024 01:56

knelli2 requested changes Jun 11, 2024

View reviewed changes

wthrowe commented Jun 11, 2024

View reviewed changes

nilsdeppe reviewed Jun 11, 2024

View reviewed changes

wthrowe added 9 commits June 12, 2024 13:29

Add structure representing Adams LTS coefficients

070f18b

Add function for generic Adams-LTS coefficients

4e56132

Use generic LTS calculation in AdamsBashforth

0aeff0c

Rewrite local-time-stepping tests

c47e483

Most of the old version assumed properties of Adams-Bashforth, even when it didn't intend to. These new tests should work for other methods as well.

Don't assume single-substep in LTS step choosing

3f09dbe

Use dynamic storage for local LTS history

ce16c22

The monotonic Adams-Moulton method requires a similar amount of history on the local side as the LTS methods generally require on the remote side.

Add LTS support to AdamsMoultonPc

a5d7645

Remove consts in some function declarations

b53d2df

As per clang-tidy.

Limit execs with control systems to monotonic LTS

75b04f2

The others LTS steppers will deadlock.

wthrowe force-pushed the predictor-corrector_lts branch from 6a63435 to 75b04f2 Compare June 12, 2024 20:42

knelli2 approved these changes Jun 12, 2024

View reviewed changes

nilsdeppe approved these changes Jun 12, 2024

View reviewed changes

nilsdeppe merged commit e37bf40 into sxs-collaboration:develop Jun 12, 2024

knelli2 mentioned this pull request Jun 26, 2024

RK and AM time steppers with Control System #5132

Closed

3 tasks

knelli2 linked an issue Sep 7, 2024 that may be closed by this pull request

Improve time-stepping with bug fixes and performance enhancements #4583

Closed

8 tasks

Add LTS support to Adams-Moulton time steppers #6043

Add LTS support to Adams-Moulton time steppers #6043

Uh oh!

Conversation

wthrowe commented May 29, 2024

Proposed changes

Upgrade instructions

Code review checklist

Further comments

Uh oh!

nilsdeppe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knelli2 left a comment

Choose a reason for hiding this comment

Uh oh!

wthrowe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilsdeppe left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants