WIP: Interface allowing ELM to drive ATS #157
Conversation
…nitialization code to protected coordinator_init function
… now; modified ELM_ATSDriver::advance to call Coordinator::advance for each single timestep
| endif() | ||
|
|
||
| ## build static lib | ||
| add_amanzi_library(elm_ats_static |
There was a problem hiding this comment.
I mean, I assume so, or you wouldn't have added this, just yuck.
| #endif | ||
|
|
||
| // allocate, call constructor and cast ptr to opaque ELM_ATS_DRIVER | ||
| ELM_ATS_DRIVER ats_create() { |
There was a problem hiding this comment.
Also, namespaces are such a good idea. Too bad C didn't think of them.
There was a problem hiding this comment.
is this void*?
Kind of. I used an opaque pointer method that's pretty common when creating portable C++ libraries. It's defined in the accompanying header file:
ats/src/executables/elm_ats_api/elm_ats_api.h
Lines 5 to 16 in a68f7b7
To the C compiler and whatever linker the external program invokes, this is a typedef to a pointer to a forward declared dummy struct. It's like a void* with a bit of type safety. To the C++ compiler/linker, it's resolved as a ELM_ATSDriver*.
I made the decision to hide the pointer behind the typedef because calling code should never dereference the ATS object. I think some people hate this, but it seems like the right call here.
| // instantiates much of Coordinator object | ||
| // it is very important that this is called | ||
| // before using Coordinator | ||
| Coordinator:coordinator_init(*plist, comm); |
There was a problem hiding this comment.
Looks like here is your cleanup? This needs to construct coordinator and then init presumably.
Can/should we change coordinator_init to just plain init? Or would that be too confusing -- there is probably also an initialize() method here which initializes PKs/etc, so maybe keeping "coordinator" here, while being redundant, makes it clear that this is initing the coordinator itself.
There was a problem hiding this comment.
coordinator_init is best, I think
|
|
||
| // require primary variables | ||
| // -- subsurface water source | ||
| S_->Require<Amanzi::CompositeVector,Amanzi::CompositeVectorSpace>(sub_src_key_, Amanzi::Tags::NEXT, sub_src_key_) |
There was a problem hiding this comment.
So typically all the above would be in the "constructor" (so coordinator_init is the "right" spot), but Require calls would be in a "setup()" method. While it makes for two function calls instead of one, I think it makes sense to keep that parallel structure.
There was a problem hiding this comment.
Do you think I should put everything that would normally be in the constructor into a private initialization function and call it from setup()? The public interface already includes setup() and initialize().
| assume evaporation and infiltration have same sign | ||
| */ | ||
| void | ||
| ELM_ATSDriver::set_sources(double *soil_infiltration, double *soil_evaporation, |
There was a problem hiding this comment.
As per our discussion the other day, this is where you would want to insert a data structure to simplify APIs? I mean, if the entries in the struct are double*, that should be fine -- no "third copy" required.
|
|
||
| // helper function for collecting column dz and depth | ||
| void ELM_ATSDriver::col_depth(double *dz, double *depth) { | ||
|
|
There was a problem hiding this comment.
Is this going to be used by ELM or by ATS? Assuming it is for ELM, isn't it easier to call the ATS mesh functions to get this info?
If it is for ATS, eventually we should have an "extruding" mesh constructor that would take dz. But that's a distant eventually...
There was a problem hiding this comment.
This function and the calling function get_mesh_info() are very much a WIP. I was never 100% clear on the requirements for these functions and don't think they were used for the earlier coupled runs.
| // setup and advance functions in the base Coordinator class | ||
| // they have the same name, but the signatures are different, | ||
| // so they aren't virtual and they don't override Coordinator | ||
| void setup(MPI_Fint *f_comm, const char *input_filename); |
There was a problem hiding this comment.
Can we add a "verbosity" flag to this as well? It would be nice to set the global verbosity (typically done through the command line) based on some ELM flag.
I guess alternatively we could add a global verbosity flag to the ATS input file, which we probably either already have or should have.
…ssed correctly, ATS pressure initialization from ELM water content, etc.
|
This is stale, new PR will be coming soon. |
This PR contains an interface that allows ATS to be compiled as a library that can be called from an external program, like ELM.