Revise Formulation_Manager.hpp

[stcui007]

Revise Formulation_Manager.hpp so that it can parse forcing params with pattern in explicit catchment configs

Additions

data/test_bmi_multi_realization_config_w_netcdf.json
data/test_bmi_multi_realization_config_w_noah_pet_cfe.json
data/test_realization_config.json

Removals

Changes

include/realizations/catchment/Formulation_Manager.hpp

Testing

Unit test
Run ngen in serial mode with various realization configure files
Run ngen in parallel mode with various realization configure files

Screenshots

Notes

Todos

Checklist

• [x ] PR has an informative and human-readable title
• [x ] Changes are limited to a single goal (no scope creep)
• [x ] Code can be automatically merged (no conflicts)
• [x ] Code follows project standards (link if applicable)
• Passes all existing automated tests
• Any change in functionality is tested
• New functions are documented (with a description, list of inputs, and expected output)
• Placeholder code is flagged / future todos are captured in comments
• Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
• Reviewers requested with the Reviewers tool ➡️

Testing checklist (automated report can be put here)

Target Environment support

• [x ] Linux

[mattw-nws]

This block seems redundant, since this kind of check/retry has already been done in check_opendir, any error that falls through from that has already timed out or is non-recoverable, so I think the check/retry loop here can be removed.

[mattw-nws]

This approach may be okay (with some tweaks... e.g. we probably don't need a regex to find {{id}} as .find() will do fine), but it seems like there is still a lot of duplication...

It seems both paths start with a JSONProperty and produce a forcing_params object... which is almost exactly what the existing get_global_forcing_params function does--but that function always operates on this->global_forcing. I think, it may be a better approach to modify this function:

forcing_params get_global_forcing_params(std::string identifier, simulation_time_params &simulation_time_config) {
    std::string path = this->global_forcing.at("path").as_string();
    // ... etc.

to be:

forcing_params get_forcing_params(geojson::PropertyMap &forcing_prop_map, std::string identifier, simulation_time_params &simulation_time_config) {
    std::string path = forcing_prop_map.at("path").as_string();
    // ... etc.

And whenever there is a call to:

forcing_params forcing_config = this->get_global_forcing_params(identifier, simulation_time_config);

make it:

forcing_params forcing_config = this->get_forcing_params(this->global_forcing, identifier, simulation_time_config);

Then, in consruct_formulation_from_tree, you could replicate what is done up at line 59 and following:

                    for (auto &forcing_parameter : (*possible_global_config).get_child("forcing")) {
                        this->global_forcing.emplace(
                            forcing_parameter.first,
                            geojson::JSONProperty(forcing_parameter.first, forcing_parameter.second)
                        );
                      }

To turn the possible_forcing into a local_forcing (not global_forcing) geojson::PropertyMap which can be passed to the same function. Then it can be passed to the same function like this:

forcing_params forcing_config = this->get_forcing_params(local_forcing, identifier, simulation_time_config);

Can you try this approach? It might be worth doing a new branch (from upstream:master ... making sure commit 444bd00 isn't included, which I think will break merging). I think this may be a much smaller/cleaner change.

[stcui007]

Let me think through this.

…

On Wed, Jan 11, 2023 at 12:53 PM Matt Williamson ***@***.***> wrote: This approach may be okay (with some tweaks... e.g. we probably don't need a regex to find {{id}} as .find() will do fine), but it seems like there is still a lot of duplication... It seems both paths start with a JSONProperty and produce a forcing_params object... which is almost exactly what the existing get_global_forcing_params function does--but that function always operates on this->global_forcing. I *think*, it may be a better approach to modify this function: forcing_params get_global_forcing_params(std::string identifier, simulation_time_params &simulation_time_config) { std::string path = this->global_forcing.at("path").as_string(); // ... etc. to be: forcing_params get_forcing_params(geojson::PropertyMap &forcing_prop_map, std::string identifier, simulation_time_params &simulation_time_config) { std::string path = forcing_prop_map.at("path").as_string(); // ... etc. And whenever there is a call to: forcing_params forcing_config = this->get_global_forcing_params(identifier, simulation_time_config); make it: forcing_params forcing_config = this->get_forcing_params(this->global_forcing, identifier, simulation_time_config); Then, in consruct_formulation_from_tree, you could replicate what is done up at line 59 and following: for (auto &forcing_parameter : (*possible_global_config).get_child("forcing")) { this->global_forcing.emplace( forcing_parameter.first, geojson::JSONProperty(forcing_parameter.first, forcing_parameter.second) ); } To turn the possible_forcing into a local_forcing (not global_forcing) geojson::PropertyMap which can be passed to the same function. Then it can be passed to the same function like this: forcing_params forcing_config = this->get_forcing_params(local_forcing, identifier, simulation_time_config); Can you try this approach? It might be worth doing a new branch (from upstream:master ... making sure commit 444bd00 <444bd00> isn't included, which I think will break merging). I think this may be a much smaller/cleaner change. — Reply to this email directly, view it on GitHub <#488 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRL4QREUISONJAZUNN3WR36S3ANCNFSM6AAAAAATXKHAUM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[stcui007]

Yeah, I'll try this out. I remember starting with a similar approach but changed to the current approach due to the complication dealing with local forcing as it is not at the same tree level as the global forcing, I think. But with passing forcing_params as an argument, it probably will work. Things will become more clearer when I start to implement it. On Wed, Jan 11, 2023 at 12:59 PM Shengting Cui - NOAA Affiliate < ***@***.***> wrote:

…

Let me think through this. On Wed, Jan 11, 2023 at 12:53 PM Matt Williamson ***@***.***> wrote: > This approach may be okay (with some tweaks... e.g. we probably don't > need a regex to find {{id}} as .find() will do fine), but it seems like > there is still a lot of duplication... > > It seems both paths start with a JSONProperty and produce a > forcing_params object... which is almost exactly what the existing > get_global_forcing_params function does--but that function always > operates on this->global_forcing. I *think*, it may be a better approach > to modify this function: > > forcing_params get_global_forcing_params(std::string identifier, simulation_time_params &simulation_time_config) { > std::string path = this->global_forcing.at("path").as_string(); > // ... etc. > > to be: > > forcing_params get_forcing_params(geojson::PropertyMap &forcing_prop_map, std::string identifier, simulation_time_params &simulation_time_config) { > std::string path = forcing_prop_map.at("path").as_string(); > // ... etc. > > And whenever there is a call to: > > forcing_params forcing_config = this->get_global_forcing_params(identifier, simulation_time_config); > > make it: > > forcing_params forcing_config = this->get_forcing_params(this->global_forcing, identifier, simulation_time_config); > > Then, in consruct_formulation_from_tree, you could replicate what is > done up at line 59 and following: > > for (auto &forcing_parameter : (*possible_global_config).get_child("forcing")) { > this->global_forcing.emplace( > forcing_parameter.first, > geojson::JSONProperty(forcing_parameter.first, forcing_parameter.second) > ); > } > > To turn the possible_forcing into a local_forcing (not global_forcing) > geojson::PropertyMap which can be passed to the same function. Then it > can be passed to the same function like this: > > forcing_params forcing_config = this->get_forcing_params(local_forcing, identifier, simulation_time_config); > > Can you try this approach? It might be worth doing a new branch (from > upstream:master ... making sure commit 444bd00 > <444bd00> > isn't included, which I think will break merging). I think this may be a > much smaller/cleaner change. > > — > Reply to this email directly, view it on GitHub > <#488 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACA4SRL4QREUISONJAZUNN3WR36S3ANCNFSM6AAAAAATXKHAUM> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

[stcui007]

Currently, it seems "global_forcing" variable is visible globally, do we have to make it a local variable to pass around as an argument? On Wed, Jan 11, 2023 at 2:02 PM Shengting Cui - NOAA Affiliate < ***@***.***> wrote:

…

Yeah, I'll try this out. I remember starting with a similar approach but changed to the current approach due to the complication dealing with local forcing as it is not at the same tree level as the global forcing, I think. But with passing forcing_params as an argument, it probably will work. Things will become more clearer when I start to implement it. On Wed, Jan 11, 2023 at 12:59 PM Shengting Cui - NOAA Affiliate < ***@***.***> wrote: > Let me think through this. > > On Wed, Jan 11, 2023 at 12:53 PM Matt Williamson < > ***@***.***> wrote: > >> This approach may be okay (with some tweaks... e.g. we probably don't >> need a regex to find {{id}} as .find() will do fine), but it seems like >> there is still a lot of duplication... >> >> It seems both paths start with a JSONProperty and produce a >> forcing_params object... which is almost exactly what the existing >> get_global_forcing_params function does--but that function always >> operates on this->global_forcing. I *think*, it may be a better >> approach to modify this function: >> >> forcing_params get_global_forcing_params(std::string identifier, simulation_time_params &simulation_time_config) { >> std::string path = this->global_forcing.at("path").as_string(); >> // ... etc. >> >> to be: >> >> forcing_params get_forcing_params(geojson::PropertyMap &forcing_prop_map, std::string identifier, simulation_time_params &simulation_time_config) { >> std::string path = forcing_prop_map.at("path").as_string(); >> // ... etc. >> >> And whenever there is a call to: >> >> forcing_params forcing_config = this->get_global_forcing_params(identifier, simulation_time_config); >> >> make it: >> >> forcing_params forcing_config = this->get_forcing_params(this->global_forcing, identifier, simulation_time_config); >> >> Then, in consruct_formulation_from_tree, you could replicate what is >> done up at line 59 and following: >> >> for (auto &forcing_parameter : (*possible_global_config).get_child("forcing")) { >> this->global_forcing.emplace( >> forcing_parameter.first, >> geojson::JSONProperty(forcing_parameter.first, forcing_parameter.second) >> ); >> } >> >> To turn the possible_forcing into a local_forcing (not global_forcing) >> geojson::PropertyMap which can be passed to the same function. Then it >> can be passed to the same function like this: >> >> forcing_params forcing_config = this->get_forcing_params(local_forcing, identifier, simulation_time_config); >> >> Can you try this approach? It might be worth doing a new branch (from >> upstream:master ... making sure commit 444bd00 >> <444bd00> >> isn't included, which I think will break merging). I think this may be a >> much smaller/cleaner change. >> >> — >> Reply to this email directly, view it on GitHub >> <#488 (comment)>, or >> unsubscribe >> <https://github.com/notifications/unsubscribe-auth/ACA4SRL4QREUISONJAZUNN3WR36S3ANCNFSM6AAAAAATXKHAUM> >> . >> You are receiving this because you authored the thread.Message ID: >> ***@***.***> >> >

[hellkite500]

Would definitely favor #489

[mattw-nws]

Cleaning up, looks like this was accomplished in #489 instead, closing.