-
Notifications
You must be signed in to change notification settings - Fork 189
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
Add LTS support to Adams-Moulton time steppers #6043
Add LTS support to Adams-Moulton time steppers #6043
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a few clarifying suggestions. Please squash and rebase immediately :)
src/Time/TimeSteppers/AdamsLts.cpp
Outdated
#include "Utilities/GenerateInstantiations.hpp" | ||
#include "Utilities/Gsl.hpp" | ||
|
||
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."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to replace it with a check that the result is correct, instead of a whitelist on the input.
src/Time/TimeSteppers/AdamsLts.cpp
Outdated
const AdamsScheme& small_step_scheme) { | ||
ASSERT( | ||
small_step_scheme == local_scheme or small_step_scheme == remote_scheme, | ||
"FIXME just deduce?"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to leave these FIXME
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebase fail. Removed altogether instead of adding in one commit and deleting later.
src/Time/TimeSteppers/AdamsLts.cpp
Outdated
(small_step_scheme.type == SchemeType::Implicit ? 2 | ||
: 1)]; | ||
ASSERT(not time_less(current_small_step, start_time), | ||
"Missed step start iterating over small steps."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
src/Time/TimeSteppers/AdamsLts.cpp
Outdated
small_step_end = current_small_step; | ||
} else { | ||
ERROR( | ||
"Multiple-small-step dense output is not supported. Split into an " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm also not entirely sure what I should change if I hit this error :(
src/Time/TimeSteppers/AdamsLts.cpp
Outdated
} | ||
|
||
// Combine duplicate entries | ||
ASSERT(not step_coefficients.empty(), "Generated no coefficients"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add "... This an algorithmic bug. Please file an issue."
?
src/Time/TimeSteppers/AdamsLts.cpp
Outdated
// 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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] alg::transform
?
src/Time/TimeSteppers/AdamsLts.cpp
Outdated
} | ||
|
||
OrderVector<double> control_times_fp(control_times.size()); | ||
std::transform(control_times.begin(), control_times.end(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] alg::transform
?
@@ -27,7 +27,8 @@ using time_steppers = | |||
Rk5Owren, Rk5Tsitouras>; | |||
|
|||
/// Typelist of available LtsTimeSteppers | |||
using lts_time_steppers = tmpl::list<AdamsBashforth>; | |||
using lts_time_steppers = | |||
tmpl::list<AdamsBashforth, AdamsMoultonPc<false>, AdamsMoultonPc<true>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself: Make the GH executables only accept the non-deadlocking time steppers in LTS mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added monotonic_lts_time_steppers
in a new commit.
e00b89b
to
9be3826
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/Time/TimeSteppers/AdamsLts.cpp
Outdated
#include "Utilities/GenerateInstantiations.hpp" | ||
#include "Utilities/Gsl.hpp" | ||
|
||
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."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to replace it with a check that the result is correct, instead of a whitelist on the input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6a63435
to
75b04f2
Compare
Proposed changes
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments