Overhaul of the initial conditions (Initial run type) #2632
Conversation
bartgol
added
enhancement
|
From the driver perspective I do think this is more straightforward to read. So in my opinion not more confusing. I do like the strategy of isolating model init to it's own code, which should make it easier to add new initialization approaches, like IOP. |
There was a problem hiding this comment.
The IOP looks like it will work fine for the current test. I added a fix we will need for PG2, but there are other things holding us back from PG2 working. Still, I think that is worth adding.
And then I added a comment about error messages, but I don't feel too strongly about that.
| auto model_init = init_factory.create(init_type,eamxx_inputs,*m_grids_manager,m_current_ts); | ||
|
|
||
| // Shorter to type | ||
| auto iop = m_intensive_observation_period; |
There was a problem hiding this comment.
We should probably just name the variable m_iop? I could change that in all the classes that have these objects in my next DP pr
| EKAT_ERROR_MSG ( | ||
| "Error! Bad name specification for constant field initialization.\n" | ||
| " - input name: " + fnames[i] + "\n"); | ||
| } |
There was a problem hiding this comment.
Is this only catching the case that std::stoi(tail) gives error? If so, should the error message mention this? I only say this because there is a high likelihood that a user reaches this error message if they are specifying constant fields. Something like
"Error! Bad name specification for constant field initialization. "
"Cannot deduce field component from specified \"cmp_"+tail+"\"\n"
" - input name: " + fnames[i] + "\n"
The same for EKAT_REQUIRE_MSG (tail.size()>0,...) above.
| auto topo_fnames_eamxx = model_init->get_topo_fields_names_eamxx(); | ||
| for (const auto& it : topo_fnames_file) { | ||
| const auto& gname = it.first; | ||
| auto fm = get_field_mgr(gname); |
There was a problem hiding this comment.
I think here we will need to insert
if (gname == "Physics PG2") {
iop->setup_io_info(filename, fm->get_grid());
}
There was a problem hiding this comment.
Looks like it's safe to call setup_io_info multiple times (the if statements should prevent code from running twice), so maybe I don't need to check the grid name? (I'm trying to keep explicit knowledge about grids names as much as possible outside the driver...)
There was a problem hiding this comment.
The issue is that if setup_io_info() is called on GLL grid with the topo file it should error out, because topo file has lat/lon for PG2 only. Maybe if GLL is already setup (it will be because ICs are read first) we will just return, but that kinda hides that it is an incompatible call.
Another option would be to change setup_io_info() to take in a list of grids that require read from file, and the IC file and topo file (if exists), and then call before ICs/topo are read. Then internally IOP can error out if an incorrect request is made (e.g., read from PG2 but no topo file) and setup every grid needed at once.
There was a problem hiding this comment.
And from the AD's perspective, they just hand over all the info they have agnostic of exactly which grids they are.
There was a problem hiding this comment.
Ok, I will think of how to best handle this.
bartgol
force-pushed
the
bartgol/eamxx/field-init-refactor
branch
2 times, most recently
from
94c575b
to
465d680
Compare
|
bartgol
force-pushed
the
bartgol/eamxx/field-init-refactor
branch
7 times, most recently
from
7e0db6c
to
c22555f
Compare
Allow registering a field with a grid name matching any of the FM grid aliases
Check if a variable is present in a file, verifying the full layout (and not just the name)
This allows some atm proc to start setting up data structures that do not depend on the actual Field values, but only on the field metadata/pointers.
* The driver relies on this class for model initialization * So far, only handle model init, not model restart * Currently, only implemented the PhysicsGLL case * PG2 and IOP will come next
* Avoid passing FieldManager objects around * Pass fields directly (no need for vectors of strings) * Check that ps,T_mid,qv are always IOP fields
bartgol
force-pushed
the
bartgol/eamxx/field-init-refactor
branch
from
c22555f
to
8deadfa
Compare
bartgol
force-pushed
the
bartgol/eamxx/field-init-refactor
branch
from
8deadfa
to
295076f
Compare
|
@bartgol can we turn this into a git-issue and tag it such that it is considered in the I/O epic? We are doing a major overhaul of I/O now and maybe that can include how we handle initial conditions? Maybe that's a stretch, but I'm concerned that this PR is now so far out-of-sync w/ master that it would be difficult to revive it from this branch. |
The initial conditions setup is one of the hackiest parts in EAMxx. The reason is that when EAMxx started, there was no PG2 support, and everything was easy: the fields to read from file are all the inputs to the atmosphere. With PG2, things got complicated, and forced us to add some hacks to get things to work.
This PR is an attempt to "hide" a bit better the hack, without using existing tools in a way that they were not meant to. In particular, rather than funneling the PG2 particular case through the non-PG2 framework (input to atm is what we read), this PR creates an interface
ModelInit(all names up for discussions, ofc) to be used for IC read. The interface is abstract, and different implementations will be used depending on whether PG2 is active or not. This allows to be more explicit in the PG2 case, without trying to shove a square peg in a round hole.So far I implemented the non-PG2 case. I tried it on a couple of non-PG2 tests (
p3_standaloneandhomme_shoc_cld_spa_p3_rrtmgp), and it seems to work (meaning that it runs to completion...I have no baselines testing for standalone scream). I will work on the PG2 case next week. Basically, the code will do the same stuff (read on physGLL, remap to dyn, then remap to PG2), but it won't have to create dummy physGLL version of the input fields and add them as "Required" fields jsut so that the AD can add them to the list of fields to read. Instead, the PG2 model init instance will directly access the IC file, and read on GLL grid, without adding any additional field to the main AD field manager.@AaronDonahue @tcclevenger this is still incomplete (there's no PG2 support!), but I'd like you to start taking a look, to see if the framework makes sense or it is even more confusing than before.
@tcclevenger I would particularly appreciate if you can take a look at how I handle IOP-vs-nonIOP in the init sequence. I hope I am doing things correctly (I haven't tried to run the dp test yet, maybe I should do that first).