-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Composition time stepper #23961
Composition time stepper #23961
Conversation
611e94b
to
aefb205
Compare
Job Precheck on aefb205 wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
aefb205
to
08ab7ed
Compare
08ab7ed
to
fb4077f
Compare
Job Documentation on a8077c4 wanted to post the following: View the site here This comment will be updated on new commits. |
Job Coverage on 579dde8 wanted to post the following: Framework coverage
Modules coverageCoverage did not change Full coverage reportsReports
This comment will be updated on new commits. |
2869ded
to
b78085a
Compare
b78085a
to
0b46ecb
Compare
Job Conda (ARM Mac) on 0b46ecb : invalidated by @MengnanLi91 |
@GiudGiud I have some basic functions done for composition time stepper and ready for review. If you have time next week, could we schedule a meeting discussing this PR and the overlay mesh generator PR? |
## Description | ||
|
||
The `CompositionDT` TimeStepper takes TimeSteppers as inputs and generates a composed time step size based on the user specific composition rules. A base TimeStepper is required to provide a baseline time step size. The available composition rules including max, min and hit. The max/min option is specified with [!param](/Executioner/TimeStepper/CompositionDT/composition_type). It produces the maximum/minimum time step size value of all the input TimeSteppers. The hit option ensures the TimeStepper hits user specified time. It requires a TimeSequenceStepper with [!param](/Executioner/TimeStepper/CompositionDT/times_to_hit_timestepper) to specify the time sequence to hit. All together, The `CompositionDT` TimeStepper allows users to have a base time step with upper or lower limits and hit specified time points. | ||
|
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.
since you are doing only one operation per CompositionDT, let s mention that we can nest CompositionDT
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 we shouldnt compose compositionDT
I think it s more simple to just have all the parameters in one?
|
||
## Example Input Syntax | ||
|
||
!listing test/tests/time_steppers/composition_dt/composition_dt.i block=Executioner |
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.
describe the example
// the name of the time sequence stepper | ||
std::string _hit_timestepper_name; |
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.
there can be more than one.
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.
It can accept multiple time sequence steppers now
Real | ||
CompositionDT::produceHitDT(const Real & composeDT) | ||
{ | ||
auto ts = setupSequenceStepper(_hit_timestepper_name); |
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.
this inits on every time step. that would be surprising?
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.
I think the function name is misleading. It is not init but gets the current TimeSequence time. I change it to getSequenceStepper
if (next_time_to_hit - _time <= _dt_min) | ||
{ | ||
ts->increaseCurrentStep(); |
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.
hitting steps should take priority over minimum time steps imo
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.
we should document that
Tuesday and Thursday might be more difficult next week, otherwise should be good |
0b46ecb
to
579dde8
Compare
@GiudGiud I've addressed most of the easy fix comments. I'll do the rest after our meeting tomorrow. |
Co-Authored-By: Guillaume Giudicelli <guillaume.giudicelli@gmail.com>
Co-Authored-By: Guillaume Giudicelli <guillaume.giudicelli@gmail.com>
579dde8
to
7336a70
Compare
Job Python spack on a8077c4 : invalidated by @MengnanLi91 |
Superseded by #24046 |
close #23740
Reason
Sometimes we want to benefit from the behavior of multiple time steppers, but save for re-implementing what s missing (for example IterationAdaptiveDT did that), you cant mix & match
Design
Introduce a timesteppersytem which allows timestepper action to create multiple timesteppers and a composition time stepper which can apply composition rules to input timesteppers to generate composed time step size and support times to hit with input TimeSequenceStepper.
Impact
Reaching the full capability of current time steppers