Formulation manager #147
Formulation manager #147
Conversation
…uld have incorrectly initialized doubles and fixed an issue where the realization manager test wasn't being read.
| "nash_n": 2, | ||
| "Cgw": 0.01, | ||
| "expon": 6.0, | ||
| "max_groundwater_storage_meters": 1.0, |
There was a problem hiding this comment.
We have some nested relationships I think we should consider. Several of parameters can be grouped and organized, i.e.
"nash_cascade": {
"name": "lateral_subsurface",
"nash_n": 2,
"nash_storage": [0.0, 0.0],
"reservoir": {
"type": "linear",
"params" : {
"Kn": 0.03 }
}
We can also treat the groudwater Reservoir similarly and lump/group these parameters together under a reservoir object i.e.
"reservoir": { "name": "groundwater",
"type": "exponential,
"parameters" : {
"max_groundwater_storage_meters": 1.0,
expon: 6.0,
Cgw: 0.01" }
}
There was a problem hiding this comment.
I would agree that representing data objects in json in manner that is closer to the c++ object layout would be beneficial. In fact it maybe worth the time to create formal interface for json serialization and de-serialization that should implemented by all data objects and then have the parsing code just call de-serialize on the input stream after the type identifier is parsed.
There was a problem hiding this comment.
Not sure why crappy noah-mp parameter names are making it into our framework. Not good. Why? They are nondescript and vague without context or units. Suggest those go away and are replaced with meaning fulnames. Happy to suggest some if you ask.
There was a problem hiding this comment.
The parameters that are in the config are just the ones in the functions themselves. The idea is that the parser reads in the JSON tree, then the formulations pick their values from the tree after everything has been created. So, a tshirt formulation is needed, it's created, then the parameters that it will feed to the actual model are grabbed from the tree. Currently, tshirt requires a parameter named expon, so it will reach into the tree and try to get a value for expon. Now, if that parameter in code changed to expon_i_dunno_im_not_the_one_with_a_masters_or_doctorate, that should also be reflected within the config as well. It's the config's job to bear data and it's the formulation's job to retrieve what it needs from it.
I think it's great news that those parameter names are crappy; it means that there seems to be a few fairly obvious places where the code can be improved. That probably means that tshirt_params, tshirt_realization, tshirt_state, etc, can all see some improvements.
There was a problem hiding this comment.
Not sure why crappy noah-mp parameter names are making it into our framework. Not good. Why? They are nondescript and vague without context or units. Suggest those go away and are replaced with meaning fulnames. Happy to suggest some if you ask.
I would suggest starting here and elaborating on the names in the documentation that those components were originally derived from. We should be able to look at the params/state structs and get a good sense of how to include this feedback into this original document.
https://github.com/NOAA-OWP/ngen/blob/master/doc/T-shirt_model_description.pdf
| virtual double get_response(double input_flux, time_step_t t, time_step_t dt, void* et_params) = 0; | ||
|
|
||
| // The neccessity of this function is in question | ||
| virtual void add_time(time_t t, double n){}; |
There was a problem hiding this comment.
This function was a specific extension only needed by HYMOD to keep the underlying model "stateless" and manage all the states in the (then) "realization".
There was a problem hiding this comment.
It's only in there so Formulation can be passed around rather than one of the two Realization classes; it's only there as a formality, really.
| return options; | ||
| } | ||
|
|
||
| virtual void validate_parameters(geojson::PropertyMap options) { |
There was a problem hiding this comment.
Definitely going to need some doc strings 😄
| }; | ||
|
|
||
| } | ||
| #endif // FORMULATION_H |
There was a problem hiding this comment.
Posix complains if you don't have new lines at the end of your files. The Atom editor dumps them in automatically to make it compliant. Might be a good idea to try to get all files ending with /n to avoid needless future diffs merely for these cosmetic/compliance reasons.
| throw std::runtime_error("No forcing definition was found for " + identifier); | ||
| } | ||
|
|
||
| geojson::JSONProperty forcing_parameters("forcing", *possible_forcing); |
There was a problem hiding this comment.
Extracting forcing within construct_formulation... implies a pretty significant linkage. This may need to be refactored a bit.
| #include <Formulation_Manager.hpp> | ||
|
|
||
| using namespace realization; | ||
| /* |
There was a problem hiding this comment.
Why is this entire file a comment?
There was a problem hiding this comment.
Linkage issues that will hopefully be fixed in the future.
| }; | ||
| }; | ||
|
|
||
| static std::map<std::string, constructor> formulations = { |
I updated some of the functionality to remove the Realization Config and Realization Config Reader so instead of looping through configs, you'll be looping through the realizations themselves. I also changed some of the terminology to Formulation instead of Realization since it's more accurate. It's not dead on, but it's a step forward.
A good bit of this is still in the process of changing. For instance, it looks like formulations will soon be components of realizations or catchments, so the class hierarchy will change and the Formulation interface will end up being merged with another class.
As a result, this PR serves as a stepping stone and proper refactoring.