As we approach a complete v1, we are realizing that some parts of the infrastructure are clunky and/or overly complicated (like adding lots of code/logic to support scenarios unlikely to ever be needed). Last night in bed I was thinking of all the overly-complicated things that can be removed, while keeping the general infrastructure and design principles of the AD intact. I'm listing them here, in fear I will forget some.
In (semi-)random order:
[x] Complete PD remapper into subfields (see #1799 ).
[ ] Make EAMxx read phis from topo file, and store it in grid. Also, convenient to store geometry data in grid as Field, since it allows to remap (p2d). This is part of bartgol/eamxx-topo-file branch.
[ ] One FieldManager for all grids. Store fields with map<string,Field>, mapping grid name to field. Still allow get_field(field_name), but only if a single version of the field is present (like in atm proc internals).
[ ] With one FM, move imported group logic inside FM class. Right now, it is in atmosphere_driver.cpp, since FM has no knowledge about other grids.
[x] Remove get_required_grids from atm procs. GM builds all supported grids. If "physics grid" is GLL, that's only "dynamics" and "physics"; if "physics grid" is PG2, that's "dynamics", "physics", and "physics GLL".
[ ] Remove remapper construction from GM. Instead, make a remapper factory with key=pair<string,string>, and register remappers when GM builds grids. This way you don't have to pass around GM just for the sake of being able to build remappers. Note: the only place that need "agnostic" remapper construction is I/O, to output dyn grid fields on physics GLL grid.
[x] Rename DynamicsDrivenGridsManager->HommeGridsManager, cause that's what it is, and it's shorter.
[ ] For field groups, have grid name in FieldGroupInfo. I think this is needed when switching back to a single FM
[ ] Move mesh free GM to "PhysicsOnlyGridsManager", inside physics/share. It's only used for system tests that do not have dynamics, so that's the proper folder. Also, no need to have this GM support SE grid.
[ ] Have physics only GM read ncols/nlevs from IC file. Right now, it's redundant/confusing that we can specify ngcols/nlevs in the input.yaml file, even if they should match the IC file.
[x] Make AD set IC/Restart file name in GM param list, so that a) homme GM can read vcoords, and b) physics only GM can read mesh sizes. Right now we have to specify a vcoord file name in the input.yaml file, but it is always the same as the IC file. Save one xml parameter to set in the defaults (and atmchange if needed).
[ ] Make mct coupling call ad.initialize (rather than individual chunks), and set sc cpl data afterwards.
[x] Remove the concept of ref grid. Always assume it's the physics grid. There's no need for this extra layer of abstraction.
[x] In homme GM, read params "Physics Grid" (possible values "GLL", "PG2") and "Physics Grid Load Balancing" (possible values "None", "Twin").
[ ] Unify get_field_in/get_field_out (and analogous for group) in AtmosphereProcess. The distinction was originally needed b/c Field was templated on scalar type (so const and non-const where different types). Now Field is no longer templated, so we can store all fields in one place.
[ ] Remove physics/share folder, and move everything into share/physics. In there, unify with content of share/util/scream_common_physics_functions and make a library (still called physics_share, or something else). The two structs are somewhat confusing: scream::physics::Functions vs scream::PhysicsFunctions. One may wonder why there are two, and what's the difference. We also don't need to keep all decl in the same struct/header. We could easily do 1 decl_header + 1 impl_header (+ ETI cpp, if appropriate) per fcn. Notice that for P3 and SHOC it makes a bit more sense to use a struct contianing all fcns, since the fcns call each other, and use lots of common types definitions. Physics fcns are somewhat independent of each other, so grouping them has no conceptual/practical purpose.
[ ] Make AtmosphereDiagnostic an independent class, that does not inherit from AtmosphereProcess. It is not a process, and the inheritance causes the diag classes to have to override some methods that they don't even need.
[ ] Fix atm DAG. First, do not mark as "missing" fields that are read from IC file (though we might consider marking them constant, somehow). Second, remove 'end of time step' node (pointless). Consider adding separate node for fields read via IC file, and never updated (like "constant IC fields"). Finally, actually use the DAG to detect unmet deps. You might also want to see if the implementation needs some revamping, which has not been touched since the early days of fields and atm procs design. Some stuff might be cleaned up. #1869
[ ] Allow AtmosphereInput to "skip" fields not found. This way, we can "try" to read every atm input, without having to pre-init constant fields. Atm procs can init uninited fields (e.g. bootstrap quantities) after the fact. Upon atm procs initialization, we can finally inspect our dag (or even just loop over atm inputs, and see if their timestamp is valid).
👍 . Reflecting on what we could have done better and making the time to improve it is really important for creating a user-friendly and bug-free code. We should definitely do these things (after Sept 1).
As we approach a complete v1, we are realizing that some parts of the infrastructure are clunky and/or overly complicated (like adding lots of code/logic to support scenarios unlikely to ever be needed). Last night in bed I was thinking of all the overly-complicated things that can be removed, while keeping the general infrastructure and design principles of the AD intact. I'm listing them here, in fear I will forget some.
In (semi-)random order:
bartgol/eamxx-topo-file
branch.get_field(field_name)
, but only if a single version of the field is present (like in atm proc internals).get_field_in
/get_field_out
(and analogous forgroup
) inAtmosphereProcess
. The distinction was originally needed b/c Field was templated on scalar type (so const and non-const where different types). Now Field is no longer templated, so we can store all fields in one place.scream::physics::Functions
vsscream::PhysicsFunctions
. One may wonder why there are two, and what's the difference. We also don't need to keep all decl in the same struct/header. We could easily do 1 decl_header + 1 impl_header (+ ETI cpp, if appropriate) per fcn. Notice that for P3 and SHOC it makes a bit more sense to use a struct contianing all fcns, since the fcns call each other, and use lots of common types definitions. Physics fcns are somewhat independent of each other, so grouping them has no conceptual/practical purpose.