Add support for short and long soil_params names in config
#100
Conversation
| > Note, calibratable parameters are passed via BMI using the _name next to the asterisk_. | ||
| > | ||
| > Slashes (/) in the variable column denote that a parameter can be specified using either name. | ||
| > For example, `soil_params.b` can equivalently be provided as `b`. | ||
|
|
||
| | Variable | Datatype | Limits | Units | Role | Process | Description | | ||
| | -------- | -------- | ------ | ----- | ---- | ------- | ----------- | | ||
| | forcing_file | *char* | 256 | | filename | | path to forcing inputs csv; set to `BMI` if passed via `bmi.set_value*()` | | ||
| | soil_params.depth | *double* | | meters [m]| state | | soil depth | |
There was a problem hiding this comment.
for consistency, we should follow the same style for soil_params.depth as well
There was a problem hiding this comment.
In an ideal world, I would have added short forms of all of the soil_params, but I couldn't for soil_params.expon because expon already exists... so, I only added short forms for calibratable parameters. @ajkhattak, thoughts on how to get around this?
There was a problem hiding this comment.
okay, I see what you mean... Let's discuss it with other folks. I would suggest something like
soil_param_name (e.g., soil_storage)
gw_param_name (e.g., gw_storage)
since the parameter expon is used in both soil and groundwater reservoirs, so following the above, we use
soil_expon and gw_expon.
we can update the names for calibratable parameters too, so they are consistent with their names in the config file.
There was a problem hiding this comment.
I am not sure if there is a need for this addition file
There was a problem hiding this comment.
Yeah, I am on the fence. I added it so you could run tests with a config that uses short form names. Also could serve as a reference for people who are setting up the module. Thoughts? Im happy to remove it if you are adamant.
| @@ -583,7 +583,7 @@ int read_init_config_cfe(const char* config_file, cfe_state_struct* model) | |||
| } | |||
| continue; | |||
| } | |||
| if (strcmp(param_key, "soil_params.smcmax") == 0 || strcmp(param_key, "soil_params.maxsmc") == 0) { | |||
| if (strcmp(param_key, "maxsmc") == 0 || strcmp(param_key, "soil_params.smcmax") == 0 || strcmp(param_key, "soil_params.maxsmc") == 0) { | |||
There was a problem hiding this comment.
we have maxsmc but not smcmax, they are used interchangeably, so better to add it as well
There was a problem hiding this comment.
Yeah, I only added maxsmc because that is the name of the calibratable parameter. I thought including a short form name would be more confusing. I guess alternatively we could change the calibratable parameter name to smcmax?
|
Thanks for the review, @ajkhattak! I'm AFK at the moment, but will get these resolved and pushed up today. |
Currently, several NOAA-OWP maintained BMI modules (see NOAA-OWP/SoilFreezeThaw#14 (comment)) share the convention of using `soil_params.<x>` where `x` is a parameter name in their config file format. However, when soil_params parameters are exposed as calibratable, the convention is to drop soil_params and just use the parameter name. To avoid confusion and increase consistency, this change enables specifying _calibratable_ cfe soil_params in config files using just their name. Support for the existing soil_params names is maintained. alpha_fc <=> soil_params.alpha_fc b <=> soil_params.b maxsmc <=> soil_params.maxsmc satdk <=> soil_params.satdk satpsi <=> soil_params.satpsi slope <=> soil_params.slope wltsmc <=> soil_params.wltsmc Klf <=> K_lf
…ss to and from BMI
aaraney
force-pushed
the
support-short-and-long-soil-params-in-config
branch
from
962da98
to
77b6c8e
Compare
|
@ajkhattak, just pushed up some changes and responded to your comments. |
Currently, several NOAA-OWP maintained BMI modules (see NOAA-OWP/SoilFreezeThaw#14 (comment)) share the convention of using
soil_params.<x>wherexis a parameter name in their config file format. However, when soil_params parameters are exposed as calibratable, the convention is to dropsoil_paramsand just use the parameter name. To avoid confusion and increase consistency, this change enables specifying calibratable cfe soil_params in config files using just their name. Support for the existing soil_params names is maintained.This PR adds support for specifying calibratable
soil_paramsusing their short form. The config readme is updated and clarifies what calibratable parameter names are used when passing to and from BMI.Changes
alpha_fc<=>soil_params.alpha_fcb<=>soil_params.bmaxsmc<=>soil_params.maxsmcsatdk<=>soil_params.satdksatpsi<=>soil_params.satpsislope<=>soil_params.slopewltsmc<=>soil_params.wltsmcKlf<=>K_lf