Open mabruzzo opened 4 months ago
I need to spend more time organizing and consolidating my thoughts on this, it's something that I've been concerned with for years. Here's a dump of my current thinking.
Cello implements an abstract "sequence of methods" for operating on data, and Enzo-E's operational behavior in a simulation is almost entirely defined in terms of the method list in the parameter file. Over the years, the combination of introducing non-physics operations into the method component (checkpoint, ordering blocks, load-balancing, flux-correction, etc.), and increasing software entropy, have increased the difficulty in specifying the method list.
I think many of the issues you mention above can be effectively dealt with by simplifying the organization of Enzo-E methods to address the technical debt. Things like merging "pm_deposit" into "gravity", renaming "pm_update" as "move_particles", having "check" and "balance" call "order" themselves as-needed, etc. In general making Enzo-E methods more "physics"- or "function"- oriented and less "algorithm"-centric.
It will necessarily break backward-compatability, so it would be a good time to introduce and use versioning parameters, and keep careful track of method modifications and the associated version.
After cleaning up Enzo-E methods, then we can shift attention to the Cello layer to address any remaining issues.
Enzo-E's foundations and Cello's overall architecture are fantastic! It does a lot of things extremely well, has a very modular design and provides a lot of conveniences! (everything @jobordner has done is truly fantastic!) I've only come to appreciate the overall design more with time (especially now that I've spent some time working with other codes).
Enzo-E's
Method
objects are arguably one of its most defining features. They have a very logical structure (initialization happens in the constructor, Block-data is updated bycompute
, timestep any requirements are computed/reported bytimestep
) that is extremely approachable (and you generally know where to look). It provides extremely useful conveniences (like the refresh-machinery). Plus it's easy to switch the machinery on and off.In my opinion, the one area that needs improvement is Method-initialization.
About Method Initialization
Currently, there is a one-to-one mapping between the
Method
objects initialized by Enzo-E and the contents of the parameter files. In more detail, the preciseMethod
classes as well as their explicit execution order are specified in the parameter-file. The logic that actually initializes the methods is performed by theProblem
class.Before I go further, let me emphasize that this is a very sensible choice (I probably would have done the same thing)! Plus, it has definitely served us well! Some of the benefits from this approach include:
What you see (in the parameter file) is what you get in the simulation. There are basically no surprises about the order of operations.
For example, imagine
Method:list
is assigned["order_morton", "balance", "mhd_vlct", "grackle"]
. The control flow executed each cycle looks likeImportantly, the challenges with this approach are not easy to foresee, and only really become problematic "at scale" (i.e. as more
Method
classes are introduced and more people designing theMethod
classes).Challenges with the current approach
In recent years, the following factors/challenges have become apparent:
Method
instances. When you have a long list of methods (8? 10?) it is easy to forget about them."order_morton"
(or"order_hilbert"
) should come before"balance"
"balance"
can't occur at the end of a cycle. It should generally come near the start"order_morton"
(or"order_hilbert"
) should come before"check"
"frame_transform"
should be the last thing"gravity"
related:"pm_deposit"
but before"pm_update"
"pm_update"
comes after"background_acceleration"
"comoving_expansion"
? Does it come after"pm_update"
"pm_update"
?)"m1_closure"
should come after hydro, but before"grackle"
"feedback"
occur in relation to"star_maker"
, hydro,"m1_closure"
"accretion"
occur in relation to"sink_maker"
, hydro?"flux_correct"
must follow hydroMethod
instance. While the system was modified a few years back to let you specify the required fields in constructors, the modified system doesn't work particularly well."grackle"
usually get initialized after the hydro-solver, the hydro-solver can't know all of the required passive-scalar fields that must be refreshedcompute
ortimestep
invocation (from a code-design perspective, that's kinda ugly. In the abs, we want to encourage people not to mutate state after initialization)"output"
method would also like to know all of the fields when it is first initialized to report errors early about desired output fields (rather than the first time it writes an output)"physics"
objects"order_morton"
and"balance"
)"m1_closure"
and"grackle"
)"flux_correct"
. To implement divergence cleaning, we would also need to inject a step after a"flux_correct"
.mhd_vlct
), but that doesn't map well across methods. We could accomplish this Involves lots of extra stepsppm
formhd_vlct
... )Physics
objects, but may not always be best solution.Importantly, most of the burden for points 1 and 2 falls on the end-user. And it's really easy to screw this up without getting any indication from Enzo-E. As the codebase grows, it will be impossible to comprehensively check for this!
How do we address this?
Honestly, I don't have all of the answers (I'm making this issue to solicit feedback). But, I have some ideas for the first few steps.
As a first step, I propose that we decouple Method initialization from the Problem class:
make
Problem
store a callback function, provided by the enzo-layer, maybe with the signature. (we may be able to avoid passingConfig&
)enzoe_setup_callback
).Problem::initialize_method
would be duplicated hereEnzoProblem::create_method_
(probably putting it in a helper function)Problem
EnzoProblem
Problem
, maybe calledProblem::call_application_setup_callback(Config&)
and have Simulation::initialize call this method (instead of callingProblem::initialize_method
).enzoe_setup_callback
a method ofEnzoProblem
, but I think the separation might be good (since the logic will eventually start deviating from the basic formula thatProblem
's otherinitialize_*
methods conform to)After that, I have a few ideas (to be tackled in subsequent PRs):
enzoe_setup_callback
in order to ensure that methods are initialized in the proper order.Method:list
parameter and build in logic to initialize the methods in a pre-defined orderpm_deposit
or a missingflux_correct
)Problem::initialize_physics
and makeenzoe_setup_callback
responsible for this (this would facilitate some EOS-related optimizations and simplifications)I very much welcome feedback. I have other ideas too.
The goal here is to pursue a solution somewhere between what Athena++ does for initializing tasks and what Enzo-E currently does. (I would prefer something a little more readable than Athena++)
What do we lose?
We lose some clarity about the order of methods?. We can work around this by passing a command line argument to Enzo-E that tells it to just print the methods.
We lose some "flexibility" of arbitrary reordering. In really, the only case where we currently have that kind of flexibility for any production sims is
"output"
dumps. In science runs, you probably want to standardize the locations (put @ start of cycle or end). This is only really a loss for scheduling debugging methods at runtime.