Stepper System #8127
Stepper System #8127
Conversation
|
Oh man - I based this on @rwcarlsen 's branch... in the meantime the |
friedmud
force-pushed
the
stepper
branch
3 times, most recently
from
89932d2
to
8e1e009
Compare
|
Epic rebase! I ended up squashing all of @rwcarlsen 's commits down into one commit... and all of my commits down into one commit. Trying to rebase ~170 commits with As it was... I had to manually copy my |
|
What about making the new steppers block stand-alone, outside the Executioner. I don't prefer the sub-blocks in general, I think the nested indentation and open/close getpot operators make the input file more cluttered. Other than this, I like the design. I haven't looked at the code yet. |
|
@aeslaughter that sounds good to me. I had the same thought myself... but I ended up keeping it this way just for consistency with the old block. Anyone else have an opinion on that? |
|
I like the ability to customize steppers in the input file. It is something I toyed with including my original PR, but ended up not adding because it seemed there was enough opposition I was hesitant to invest the time. Other than that, my (obviously biased) opinion is that this is not much of an improvement. But that ship has sailed. A few big-picture thoughts/questions from my first look (more to come later):
|
|
|
||
| protected: | ||
| /// The ExodusII file that is being read | ||
| std::string _mesh_file; |
| @@ -101,7 +108,7 @@ Transient::Transient(const InputParameters & parameters) : | |||
| _at_sync_point(declareRecoverableData<bool>("at_sync_point", false)), | |||
| _first(declareRecoverableData<bool>("first", true)), | |||
| _multiapps_converged(declareRecoverableData<bool>("multiapps_converged", true)), | |||
| _last_solve_converged(declareRecoverableData<bool>("last_solve_converged", true)), | |||
| _last_solve_converged(/*declareRecoverableData<bool>("last_solve_converged", */true/*)*/), | |||
There was a problem hiding this comment.
Actually need to remove the dead code there too. Good catch.
| auto & steppers = stepper_warehouse.getActiveObjects(); | ||
|
|
||
| // Grab the output name for the last one | ||
| auto last_name = steppers.back()->outputName(); |
There was a problem hiding this comment.
Probably should assert here. Yes, I know we call init above but it's just smart to be defensive in case something changes in the future.
| auto params = _app.getFactory().getValidParams("LimitStepper"); | ||
| params.set<StepperName>("incoming_stepper") = last_name; | ||
| _console<<"dtMin(): "<<dtMin()<<std::endl; | ||
| _console<<"dtMax(): "<<dtMax()<<std::endl; |
There was a problem hiding this comment.
Spacing around binary operators.
There was a problem hiding this comment.
Those are actually print statements that should come out anyway!
|
|
||
| ConstantStepper::~ConstantStepper() | ||
| { | ||
| } |
There was a problem hiding this comment.
Let's get in the habit of dropping empty dtors in non-base classes.
There was a problem hiding this comment.
Will do - I meant to remove all of these. I'm not sure why they crept into my code here... they just came from whatever MooseObject I happen to copy from to create the first Stepper ;-)
| #define KNOTTIMESSTEPPER_H | ||
|
|
||
| #include "Stepper.h" | ||
| #include "LinearInterpolation.h" |
| #define LIMITSTEPPER_H | ||
|
|
||
| #include "Stepper.h" | ||
| #include "LinearInterpolation.h" |
| #define POSTPROCESSORSTEPPER_H | ||
|
|
||
| #include "Stepper.h" | ||
| #include "LinearInterpolation.h" |
| const Real & _input_dt; | ||
|
|
||
| /// Number of steps in the same direction | ||
| int _n_steps; |
There was a problem hiding this comment.
Probably should be unsigned unless that causes a sign issue later in computation.
| @@ -0,0 +1,113 @@ | |||
| #ifndef STEPPERINFO_H | |||
I think I do too.
I completely disagree with this. Checking for convergence... and for initial are both somewhat complicated subjects. It's not something Steppers should be doing... nor is it something we want spread through user code. We want it in one place (like it is in this PR) so that we can change it internally as we refactor things (like the Executioners) and the Stepper code is completely agnostic to those changes. In addition... all of the This also allowed for removing most of the "branching" that was present in your PR. If we're computing "initial" DT we don't need to go through a whole huge graph of composed objects... we just need to go straight down the "initial" branch... which is what this does without all of the complication on the user's end. Just compare the number of objects that need to be created in this PR and yours...
No - this is not a
Unit testing is good when possible... and I think it should be doable here. Just need to create an FEProblem and fill up
It is already addressed. You can see it in action here: https://github.com/idaholab/moose/pull/8127/files#diff-206dbf39c28026b828236e2cf0e33314R44 That said: we shouldn't do it much. It should only be done when it would be SUPER inconvenient to put multiple blocks in the input file. For instance, the
The "no root producer" case is not possible. All of the pure "filtering" steppers have required parameters for the Stepper they couple to... so if you try to just provide a filter you will get an immediate error from the parser. Multiple root producers is perfectly fine... the actual check should be about multiple endpoints. I'll add that to the TODO list. @permcody can the DependencyResolver do this automatically?
Not true. All of the Steppers need to reference the same data. That data has to live somewhere. It's better to have it in a
I agree. I'll add it to the TODO
Yeah - I have this in my TODO above. I would actually like to make dt have:
I don't understand your graphs. It looks like you have multiple endpoints... that means multiple, independent, dts will be produced. That doesn't make any sense. Maybe your arrow are backwards from the arrows in my mind? Are you drawing them as "A -> B" means "A depends-on B" or as "A provides-to B"? (The latter one is how I'm thinking of it). It doesn't matter though. This PR allows you to create any tree structure you want. You can have multiple producers each with their own filters and then tie them back together through a final filter if you want. Steppers can couple in as many Steppers as you like... so anything is possible. It's actually much more flexible that your composition system. In your system you had to often compute things twice because you need to use the same values down multiple branches. For instance: and In this system... since it's a graph one node can be consumed by multiple other nodes. So you can have a producer that you filter in two different ways and then tie the results back together at the end. BTW: This is all theoretical because none of our current Steppers require anything like this. By eliminating all of the |
| Real | ||
| DT2Stepper::computeFailedDT() | ||
| { | ||
| // This isn't complete |
There was a problem hiding this comment.
It means that I haven't tested the failure modes of DT2 yet. I need to make sure that it unwraps properly if you fail in each of the 3 different solves. I'll add that to the TODO.
|
On Mon, Nov 28, 2016 at 10:52 AM, Derek Gaston ***@***.***> wrote:
I agree with @aeslaughter <https://github.com/aeslaughter> on avoiding
the block nesting.
I think I do too.
I don't like pulling the converged/not handling away from steppers (i.e.
separating the stepper interface into computeDT and computeFailedDT. It
makes writing new steppers more confusing, and it is harder to follow
control flow logic when looking at a stepper. I think things like
mooseError("you can't not converge for constant dt") hint that it is
probably not the best approach. I think it would be better to have the
branching be a stepper layer.
The same goes for computeInitialDT - pull it into its own stepper layer
rather than have it complicating the Stepper interface.
I completely disagree with this. Checking for convergence... and for
initial are both somewhat complicated subjects. It's not something Steppers
should be doing... nor is it something we want spread through user code. We
want it in one place (like it is in this PR) so that we can change it
internally as we refactor things (like the Executioners) and the Stepper
code is completely agnostic to those changes.
In addition... all of the converged() checks in your PR generated the most
mess. Because of your composition you had to generate the same values twice
down multiple branches and then tie them back together. It was messy and
hard to follow. By having very explicit virtual functions that do *one*
thing and do it well... things are more clear. Not too mention that this is
similar to how TimeSteppers were originally so the interface is familiar.
Most steppers don't care about convergence, and most never called
converged(). I prefer the other approach, but we can agree to disagree.
This also allowed for removing most of the "branching" that was present in
your PR. If we're computing "initial" DT we don't need to go through a
whole huge graph of composed objects... we just need to go straight down
the "initial" branch... which is what this does without all of the
complication on the user's end. Just compare the number of objects that
need to be created in this PR and yours...
In my PR, for an initial dt, the call stack was far shorter - only the
top-level wrapping initial stepper would be called - it omits calls to its
children until past the "initial" condition.
I think things like mooseError("you can't not converge for constant dt")
hint that it is probably not the best approach.
No - this is not a not implemented! or something like that... it's an
actual error for a condition that needs to be checked. Even in your system
you would have to have a convergence check and throw an error here.
Fair enough.
I really think it would be super valuable to keep unit testing. Unit tests
could replace a lot of input-file tests that are terribly slow in
comparison and provide much less targeted feedback about failures.
Unit testing is good when possible... and I think it should be doable
here. Just need to create an FEProblem and fill up StepperInfo and call
FEProblem::computeDT(). You can see that this is on my TODO list above.
What is your plan for supporting canned pre-composed steppers? This
doesn't seem to be addressed here.
It is already addressed. You can see it in action here:
https://github.com/idaholab/moose/pull/8127/files#diff-206db
f39c28026b828236e2cf0e33314R44
I see.
That said: we shouldn't do it much. It should only be done when it would
be SUPER inconvenient to put multiple blocks in the input file. For
instance, the KnotTimesStepper I just linked to takes a list of times. We
want to enforce that those times are perfectly hit so we want to use a
FixedTimesStepper to do that. However: doing it with two blocks in the
input file would be incredibly inconvenient because you would need to have
a complete *copy* of the times list in each block. By creating a
sub-Stepper like this it is automatic.
What about error handling - when users specify more than one root producer
(or none)? Just something to think about.
The "no root producer" case is not possible. All of the pure "filtering"
steppers have *required* parameters for the Stepper they couple to... so
if you try to *just* provide a filter you will get an immediate error
from the parser.
Cool.
Multiple root producers is perfectly fine... the actual check should be
about multiple *endpoints*. I'll add that to the TODO list. @permcody
<https://github.com/permcody> can the DependencyResolver do this
automatically?
Yeah - we have roots and leaves in reverse in our heads :)
If you are just going to fan out StepperInfo's fields directly into
Stepper, then there is no need for StepperInfo any more. It should either
be a field on Stepper or should be eliminated.
Not true. All of the Steppers need to *reference* the same data. That
data has to live somewhere. It's better to have it in a StepperInfo
object than to put that data directly into FEProblem. If we were going to
eliminate something I would say eliminate the info from Transient. I'll add
that to the TODO.
Makes sense - then I agree we should keep it.
I'd like to see many of the Stepper fields be const and private.
I agree. I'll add it to the TODO
Also, ones that have history feel better as functions for access (e.g.
dt() instead of _dt[0]).
Yeah - I have this in my TODO above. I would actually like to make dt
have: _dt, _dt_old and _dt_older to mirror how things are done elsewhere.
But I don't have a really strong feeling for that.
It looks like support for generic tree stepper composition has been
removed - you can only create trees that have arbitrary branching
underneath through one child. I can no longer have:
A -> B
A -> C
B -> D
B -> E
C -> F
C -> G
Instead I am limited to:
A -> B
A -> C
C -> D
C -> E
where B cannot have a sub-tree.
I don't understand your graphs. It looks like you have multiple
endpoints... that means multiple, independent, dts will be produced. That
doesn't make any sense. Maybe your arrow are backwards from the arrows in
my mind? Are you drawing them as "A -> B" means "A depends-on B" or as "A
provides-to B"? (The latter one is how I'm thinking of it).
A -> B means A depends on B.
It doesn't matter though. This PR allows you to create any tree structure
you want. You can have multiple producers each with their own filters and
then tie them back together through a final filter if you want. Steppers
can couple in as many Steppers as you like... so anything is possible.
It's actually much more flexible that your composition system. In your
system you had to often compute things *twice* because you need to use
the same values down multiple branches.
Yes - this was a weakness of my approach that I wasn't fond of - but in
reality I would expect this code to be so far from a "hot spot" that it
probablly wouldn't matter much in practice. The flip side though is that
steppers need to be coded carefully so that they can correctly handle being
called multiple times for the single simulation dt computation.
For instance:
https://github.com/rwcarlsen/moose/blob/f514a3a567404c9dfe88
840e3da997a53aa2e799/framework/src/timesteppers/ConstantDT.C#L41
and
https://github.com/rwcarlsen/moose/blob/f514a3a567404c9dfe88
840e3da997a53aa2e799/framework/src/timesteppers/FunctionDT.C#L46
In this system... since it's a graph one node can be consumed by multiple
other nodes. So you can have a producer that you filter in two different
ways and then tie the results back together at the end.
BTW: This is all theoretical because none of our current Steppers require
anything like this. By eliminating all of the converged() checking and
doing min() inside the filters it completely straightened out a lot of
the branching... leading to MUCH simpler flow. Just look at how many
objects your Steppers had to use to get something done vs the one or two
that are now used in this PR.
It still isn't clear to me how the min() is implemented/used - could you
elaborate a bit on this? And now I can see how tree/graph building would
look - the limitation indeed does not exist in this implementation.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8127 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABSeSkjEvmkfMihqa73ZMIYgyAKTSFeNks5rCxTugaJpZM4K9UVc>
.
|
Not a performance issue (although, theoretically it could be, or it could even be a memory issue if you had to read in a large data structure (like an Exodus file) in multiple branches). Mostly: it just makes things more complicated than they need to be. Our users would have to actually think about how they would need to generate multiple copies of values to pass down different different branches... yuck. I guess you would say it's a "code smell" 😄
Not in this PR. Every Stepper is only called once... no matter how many times their output value is consumed. |
The |
Right [facepalm] - not sure what I was thinking when I wrote that... |
The For instance... look at In your system you would have had two branches and tied them back together with a |
|
This doesn't replace current time-stepper implementations - it just provides a new architecture along side. I think it would really help the executioner refactoring effort (regardless of who does it) if we didn't have to tip-toe around the existing time stepper implementations. Just my 2 bits. |
I haven't removed the current TimeStepper yet. There will be a deprecation period (a short one for some of them, sightly longer for others) where we get people to change their inout files over to the new system. We will also quickly add in a The Executioner refactor will take way longer than the deprecation period. It's not even clear that we are going to do an executioner refactor right now. But that's a conversation for a different thread. |
|
I know that @jwpeterson really hates #if 0. This particular one isn't as
bad since you also commented out the bodies. The issue is it can be tricky
to see code that looks like it should otherwise be compiled because of a
tricky and easy to miss preprocessor directive.
I would suggest that you keep the structure of this file and allow it to be
compiled even if the bulk of the code is still commented out.
…On Mon, Nov 28, 2016 at 2:22 PM Derek Gaston ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In unit/src/StepperTest.C <#8127>:
> @@ -0,0 +1,334 @@
+/****************************************************************/
+/* DO NOT MODIFY THIS HEADER */
+/* MOOSE - Multiphysics Object Oriented Simulation Environment */
+/* */
+/* (c) 2010 Battelle Energy Alliance, LLC */
+/* ALL RIGHTS RESERVED */
+/* */
+/* Prepared by Battelle Energy Alliance, LLC */
+/* Under Contract No. DE-AC07-05ID14517 */
+/* With the U. S. Department of Energy */
+/* */
+/* See COPYRIGHT for full restrictions */
+/****************************************************************/
+
+#if 0
I have some intention of keeping *some* of the unit tests. What do you
want to do with it for now? We can always go dig them out of @rwcarlsen
<https://github.com/rwcarlsen> 's original commit (which is preserved on
this branch).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8127>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AC5XIKxXXbSiJ4Hd4V_giQV178DRlPqsks5rC0YmgaJpZM4K9UVc>
.
|
|
I think I would rather delete it then comment it out... it will be a pretty large chunk of commented code. We can always resurrect it later... |
|
Atom grays out |
|
How hard would it be to work in adaptive time stepping based on integration error estimators into this system? |
|
Very easy I would think. I mean: the The great part about this new system is that you can code up your adaptive time stepping scheme... and then easily combine it with several others. For instance, from the input file, you can add your adaptive |
|
Ok - rebased this and repushed it. Let's see what the testing system says about it... |
|
@permcody Ok - the only thing that is failing is 2 tests in Yak. I'm taking a look at it now to see if I can spot the issue. That said: Everything is not using the new system yet. With this PR I left the old system so we can transition. A bigger task is to work through all of the input files and switch them over to the new system and get them working, etc. I'm really not going to have time to do that... that's going to have to be an ongoing effort for a while. We definitely can make it so that when you try to use the old system it actually invokes the new one. I've held off on that for now because I want to give us time to do more testing... but eventually it will be the right thing to do (for a while, until we remove the old syntax all together). |
|
@YaqiWang I just looked at those tests... and the new behavior looks to be more correct than the old behavior! For instance: in Can you take a look at those couple of tests that are failing here and either regold them... or, if you think they are right like they are, tell me why? Thanks! |
|
I know I am not sure if the gold file are correct or not with the old DT2. We can disable it for now. DT2 implementation needs to be verified anyway. |
|
@YaqiWang - If you are willing to disable them, let's go with that. Then we can review this PR and get it merged. |
|
OK, a MR is sent in Yak. BTW, the other failing test is due to file naming conflict. It is irrelevant with this PR. It should have been fixed. |
|
@idaholab/moose-team - ready for review... |
|
Job |
|
Migrating to #8554. |
This PR aims to be a reimagining of #7849. I wanted to keep the core idea of #7849 (small algorithms feeding each other), but do it in a way that is familiar and natural within MOOSE. In addition to reusing some code by @rwcarlsen in #7849, I took inspiration from comments from @aeslaughter ( #7849 (comment) ) and @dschwen ( #7849 (comment) ) along with some of my own: #7849 (comment) and #7849 (comment) .
I had a few goals in mind as I crafted this PR:
valid_params()I believe I’ve met these objectives. The result is something fully MOOSE-like.
What I’ve arrived at uses a directed, acyclic graph (DAG) of small algorithms that feed
dtthrough each other. These new objects are calledSteppers. They are wholly separate fromTimeSteppers(TimeStepperswill be deprecated and eventually removed). Steppers can consumedtfrom other Steppers by coupling to them (callinggetStepperDT()throughStepperInterface… like normal).A large part of the complexity of #7849 came from the mechanism for combining algorithms: tiny “combiner” algorithms (some as small as a single
ifstatement). To keep this out of my current design I’ve adopted a different information flow: “filtering”.dtis “produced” by some Steppers… while others take in thatdtand “filter” it (often constraining it). A graph of these produces the final DT.This means that
Stepperscan generally be broken into two categories: Producers and Filters. There is no actual distinction in the API (someStepperscan be both). Here are some examples of each:Producers:
SimpleStepper(essentially what used to beConstantDT)ConstantStepper(actually constant! Errors out if a solve fails!)IterationAdaptiveStepper(the key adaptivity algorithm fromIterationAdaptiveDTthat @rwcarlsen pulled out in Refactoring and cleaning up time steppers #7849)PiecewiseStepper(similar to the oldFunctionDT- which didn’t use a Function!)PostprocessorStepperFilters:
dtat partcular times)(BTW: I’m open to changing these names… let’s try to get them right while we have the chance!)
(Note: I would like to make it clear that much of the code that ended up in these
Stepperscame from @rwcarlsen in #7849. Like I said: there was some really good code in there... just wrapped up in the wrong implementation).Using these it’s simple to dial up many of the things our users have asked for over the years. For instance, you can now use the normal time stepper (
SimpleStepper) but choose a few points in time to hit perfectly:This used to only be possible using the
Outputsystem’s “sync times” or by abusingIterationAdaptiveDT. It’s now really straightforward to do right in the input file. This is how I believe 99% of our users will interact with the system. They will pick a “Producer” that is close to what they want and then use “Filters” to tailordtto their needs.Speaking of
IterationAdaptiveDT… it grew into an abomination… it was actually several timestepping algorithms smashed together (you could even turn off the adaptivity!). The only reason it existed like this is because, until now, users could only choose one object fordtcomputation. So,IterationAdaptiveDTjust had more and more stuff thrown into it so that you could choose different combinations of algorithms. When most people usedIterationAdaptiveDTthey would use the core timestep adaptivity algorithm… and then typically select one more modifcation to that. This is now very simple to do in the new system so there is no need to have a massive stepper object likeIterationAdaptiveDT.For instance, what used to be:
[./TimeStepper] type = IterationAdaptiveDT dt = 1.0 optimal_iterations = 10 time_t = '0.0, 5.0' time_dt = '1.0, 5.0' [../]Is now:
[./Steppers] [./adapt] type = IterationAdaptiveStepper dt = 1.0 optimal_iterations = 10 [../] [./knots] type = KnotTimesStepper incoming_stepper = adapt times = '0.0, 5.0' dts = '1.0, 5.0' [../] [../]The
IterationAdaptiveStepperis a “Producer” and then you can “Filter” it in any way you want to achieve your desired goal. While it seems there is a bit more to do... it's actually easier to use because the options for each object are greatly paired down instead of being confronted with a wall of (often contradictary) options like inIterationAdaptiveDT.For instance, the
iteration_adaptive/hit_function_knottest used to have this in it:[./TimeStepper] type = IterationAdaptiveDT dt = 0.9 optimal_iterations = 10 force_step_every_function_point = true max_function_change = 1e20 timestep_limiting_function = knot [../]It's actually abusing the
IterationAdaptiveDTobject to just solve at specific times. It now looks like this:[./Steppers] [./fixed] type = FixedTimesStepper times = '0 1 2' [../] [../]Instead of abusing an object... you just dial up exactly what you want.
In addition, this way of working is perfectly MOOSE-like. For instance, mirroring the way
MeshModifierswork one after another to produce the final mesh.Inside of a
Stepperthings are also greatly simplified over what was in #7849. Instead of the mess inmoose/framework/src/timesteppers/ConstantDT.C
Line 41 in f514a3a
Real SimpleStepper::computeDT() { return _prev_dt = std::min(_input_dt, _growth_factor * _prev_dt); }So
SimpleSteppercan live up to its name!There are still a few things left to do... but I'm out of time for now. Here are a few things:
PredictorCorrectorStepperConstantDTbeing deprecated soon... then progress toward finally deprecatingIterationAdaptiveDTTimeSteppers(I did a bunch, but I'm out of time)backup()andrestore()methods to make sure they are cleaning up old backups_dtinSteppers(right now access is just_dt[0]. Refactoring and cleaning up time steppers #7849 used a function which might be a good idea. I'm up for discussion.Stepperto be more in line with MOOSE: i.e._time->_t, etc.FEProblem::computeDT()are. They are currently inFEProblembut I could be swayed to put them inTransient.Transient::lastSolveConverged()and how convergence is found/stored in general. It's kind of a mess.isParamValid()is being used... but we should really addStepperInterface::isStepperCoupled()or something similar.numeric_limits::max()... which does the right thing for all of our currentSteppers. However, we should allow the specification of default coupling (including the specification ofincoming_steppervalue right from the input file... just as with Postprocessors, Functions, etc.DT2Stepper::computeFailedDT()SteperInfo... currently that data is duplicated inTransientand inFEProblem::_stepper_info. It should really only live in the object. Most likely that object needs to be restartable/recoverable.Steppermember variables should beconstreferencesI'll update this list as we generate more TODOs through discussion.