Open YaqiWang opened 3 years ago
This flexible executioner would end up having to support the same functionalities as one of eigenvalue/steady/transient, ending up in major code duplication, way beyond what you proposed by just moving the solve object creation down one level. Maintenance and testing costs to support having this parallel system are probably higher than we would like.
I could see some version of this simply be the main design for executioners though, especially the part with nesting or ordering solve objects. This would be a really major rework but would add some pretty major new customizing capability. (technically these capabilities are already there, but through coding custom executioners and solve objects, not through just using what s already there from the input file).
I still think the creation of solve objects in the executioner base class is not a real issue, so hopefully this is not the driver for this proposed new design.
I'll let @lindsayad comment on whether AMR/grid stuff could be a solve object / what type of object/construct it could be. We dont need to have everything be solve objects, we just need the nesting capability and a lot of flexibility.
@friedmud This is the issue we were referring to. To be clear, we are not in urgent needs of this for Griffin.
My concern is more about naming, not about implementation. If AMR and time steppers are SolveObjects
, then are we ever doing something other than solve objects? Should we see if we can change int main()
to SolveObject
? When deciding upon a mesh and a time step size, to me that's more akin to a SetupObject
in terms of name. We are setting up the parameters of the simulation that we are going to eventually solve. I'm on board with lots of different things deriving from *Object
. I just wouldn't like looking at a code base in which everything is a SolveObject
.
On the other hand we do already have a SolveObject
and if during the course of solving this issue there never comes a need to make any more layers of abstraction, then I am fine with everything being a SolveObject
.
So after the meeting: wrt to executioners:
wrt to solve objects:
wrt to other executioner objects:
wrt to other executioner objects:
* should they all be solve objects? I m not sure
As I mentioned in the call, we made a deliberate decision to prefer Adaptivity
syntax outside of the Executioner
. I can't actually say why that decision was made as I wasn't here at the time... but whatever the reasoning was, that choice suggests to me that Adaptivity
is not an "executioner object".
Let's compile a complete list of executioner objects we want supported and see if there s some patterns. Supporting nesting with objects that are defined outside of the executioner might be harder?
not an "executioner object".
I said that my concerns are about naming, but this statement that I made was actually about implementation (e.g. most Adaptivity
simulations do not have syntax in the Executioner
block). I should stick to my naming concern because that's what matters to me.
I do actually like a name like ExecutionObject
and I think that adaptivity and time-stepping fit under such a name.
I whole-heartedly agree that we want to support clean nesting of "execution" objects.
My concern is more about naming, not about implementation. If AMR and time steppers are
SolveObjects
, then are we ever doing something other than solve objects? Should we see if we can changeint main()
toSolveObject
?
No, we will still have actions for setting up which is more like SetupObject
you are referring. SolveObject
will have its constructor, init
and preProblemInit
for its related setups.
So after the meeting: wrt to executioners:
- the initialization of the fixed point solve and the FEProblemSolve should be done in a routine so it can be overwritten by child classes
- adding the parameters of the fixed point solve and the FEProblemSolve should also be done that way (this is a later though, @YaqiWang what do you think of this) so they can be overwritten
The code is simple enough like
_feproblem_solve(*this)
_fixed_point_solve(*this)
_fixed_point_solve.setInnerSolve(_feproblem_solve)
params += FixedPointSolve::validParams()
so I do not think providing extra functions is necessary.
- the logic of handling solve object nesting needs to be in Executioner, because we need Steady/Transient/Eigenvalue to inherit it as well
No. Whoever finishing construction of all solve objects, whoever set up the nesting by calling SolveObject::setInnerSolve
.
wrt to solve objects:
- they should be in the factory, and within their blocks you want to be able to specify either their inner solve or their parent solve
Minor: they should only have inner_solve
, which is sufficient for the chain.
- the old way of creating them, additional params to Executioner should remain valid. This should still be handled by a base class of Steady/Transient/Eigenvalue to avoid code duplication. Ideally, this is Executioner.
wrt to other executioner objects:
- should they all be solve objects? I m not sure
If we have the factory for setting up all solve objects and adding the nesting, we do not need an executioner. But we still have Steady
, Eigenvalue
and Transient
for fixed setups of solve objects for normal users.
- should we enable arbitrary nesting? To some extent. Like the innermost solve can't be a grid refinement object for example.
Some solve objects always do not have inner solves like FEProblemSolve
. They can optionally have no inner solves. But currently PicardSolve
is made an inner solve as required (we can change the code easily to make it optional though).
The code is simple enough like
_feproblem_solve(*this) _fixed_point_solve(*this) _fixed_point_solve.setInnerSolve(_feproblem_solve) params += FixedPointSolve::validParams()
so I do not think providing extra functions is necessary.
If you dont provide extra functions, then you're not going to be able to do what you want (move FEProblemSolve initialization) without code duplication.
No. Whoever finishing construction of all solve objects, whoever set up the nesting by calling
SolveObject::setInnerSolve
.
Steady will not be able to do the nesting if you apply this principle. This needs to be done in a base class.
Minor: they should only have
inner_solve
, which is sufficient for the chain.
yep that works. Parent solve works just as well. Meshgenerator use more of the latter than the former.
If we have the factory for setting up all solve objects and adding the nesting, we do not need an executioner. But we still have
Steady
,Eigenvalue
andTransient
for fixed setups of solve objects for normal users.
You need an executioner to create the solve objects. They will be defined in the executioner block. I m not thinking they should be entirely their own block.
Some solve objects always do not have inner solves like
FEProblemSolve
. They can optionally have no inner solves. But currentlyPicardSolve
is made an inner solve as required (we can change the code easily to make it optional though).
You can change that by moving the current settings into member functions then overriding them for your special cases.
I am working on this because I realized that I need this to make the very complicated executioner in Griffin clean. I have something working but need to add more tests and fix Griffin accordingly. Should push up something very soon.
Reason
With the current design of our executioner system with solve objects, we are able to create a flexible executioner that can dial up various solve objects for the advanced users. These users can custom the calculation to have AMR or not, Picard iteration for multiphysics or not, etc.
Design
We can create a new executioner type like
FlexibleExecutioner
which takes solve objects with a parameter likesolve_object_sequence
added through factory with an input syntax. For example, after we can make the AMR loop as a solve object, the following input:will basically be doing what
Steady
is doing. Note that to make this work, Executioner should not be adding any solve objects. [Syntax is updated based on our discussions.] We can call this the factory-way of adding solve objects.Our current way of adding solve objects is
with which
Steady
add the three solve objects and set the pre-determined nesting properly.Another way of adding solve objects is
We should keep what executioners we have currently for normal users.
One remaining issue is to make the output system to support this flexible executioner. I propose to extend
SolveObject
to have a function likeoutputTime()
andoutputDT
, which will be made visible to the inner solver object that can be used for outputting. For example, ifPicardSolve
object is wrapped insideTimeStepper
,PicardSolve
can use the following formula to evaluate its current output time and dt asSolve objects can add custom execution flags like
EXECUTE_PICARD_BEGIN
, etc.Another issue I can see is restart/recover, which we possibly should make available only for timesteeping.
Impact
A new capability. This requires some improvements on outputting, converting some codes such as timesteppers, AMR, into solve objects, which can make current executioners better maintained.