From 825154a930d81dc4d5c414bc753f911dd0592164 Mon Sep 17 00:00:00 2001 From: program-- Date: Tue, 23 May 2023 14:33:04 -0700 Subject: [PATCH 1/3] support user-directed output paths Squash 2d8cff1 and 9a0b5f5 --- doc/REALIZATION_CONFIGURATION.md | 6 ++ .../catchment/Formulation_Manager.hpp | 74 +++++++++++++++++++ src/NGen.cpp | 5 +- src/core/HY_Features.cpp | 2 +- src/core/HY_Features_MPI.cpp | 2 +- .../realizations/Formulation_Manager_Test.cpp | 6 ++ 6 files changed, 91 insertions(+), 4 deletions(-) diff --git a/doc/REALIZATION_CONFIGURATION.md b/doc/REALIZATION_CONFIGURATION.md index 77b670ea85..a52e73b983 100644 --- a/doc/REALIZATION_CONFIGURATION.md +++ b/doc/REALIZATION_CONFIGURATION.md @@ -31,6 +31,8 @@ The `global` key-value object must contain the following two object keys: * `forcing` * key-value object with keys for `file_pattern` and `path` that define the default CSV file pattern and path for the input forcings relative to the executable directory +The `global` key-value object *may* contain an `outputs` object key for user-defined output patterns for nexus and catchment outputs, where the `{{id}}` syntax reflects the nexus or catchment string identifiers (i.e. `cat-27`, `nex-68`). + ``` "global": { "formulations": [ @@ -46,6 +48,10 @@ The `global` key-value object must contain the following two object keys: "forcing": { "file_pattern": ".*{{id}}.*.csv", "path": "./data/forcing/" + }, + "outputs": { + "nexus": "/path/to/nexus/{{id}}-output.csv", + "catchment": "/path/to/catchment/{{id}}-output.csv" } }, ``` diff --git a/include/realizations/catchment/Formulation_Manager.hpp b/include/realizations/catchment/Formulation_Manager.hpp index 2c947767f6..88279cc8b8 100644 --- a/include/realizations/catchment/Formulation_Manager.hpp +++ b/include/realizations/catchment/Formulation_Manager.hpp @@ -250,6 +250,80 @@ namespace realization { return ""; } + /** + * @brief Get the formatted nexus output path with the given @c id interpolated + * + * @code{.cpp} + * // Example config: + * // ... + * // "outputs": { + * // ... + * // "nexus": "/path/to/nexus-dir/{{id}}-output.csv", + * // ... + * // } + * // ... + * const auto manager = Formulation_Manger(CONFIG); + * manager.get_nexus_output_path("nex-27"); + * //> "/path/to/nexus-dir/nex-27-output.csv" + * @endcode + * + * @param id Nexus string ID + * @return std::string of the interpolated nexus output path if the realization config file + contains it, otherwise an output path in the form "./{{id}}_output.csv". + */ + std::string get_nexus_output_path(const std::string& id) const noexcept { + const auto nexus_path_format = this->tree.get_optional("global.outputs.nexus"); + if (nexus_path_format != boost::none) { + const auto pattern_idx = nexus_path_format->find("{{id}}"); + if (pattern_idx != std::string::npos) { + std::string nexus_path = *nexus_path_format; // copy + return nexus_path.replace(pattern_idx, sizeof("{{id}}") - 1, id); + } + // TODO: should we still return the path if it doesn't have an ID parameter? + // this would imply that the user specified a path (and may know) that will + // only ever output 1 file. Since, if there is more than 1 output, they will + // overwrite one another. + } + + // TODO: Should catchments and nexi have the same default output format? + return "./" + id + "_output.csv"; + } + + /** + * @brief Get the formatted catchment output path with the given @c id interpolated + * + * @code{.cpp} + * // Example config: + * // ... + * // "outputs": { + * // ... + * // "catchment": "/path/to/catchment-dir/{{id}}-output.csv", + * // ... + * // } + * // ... + * const auto manager = Formulation_Manger(CONFIG); + * manager.get_catchment_output_path("cat-27"); + * //> "/path/to/catchment-dir/cat-27-output.csv" + * @endcode + * + * @param id Catchment string ID + * @return std::string of the interpolated catchment output path if the realization config file + contains it, otherwise an output path in the form "./{{id}}.csv". + */ + std::string get_catchment_output_path(const std::string& id) const noexcept { + const auto catchment_path_format = this->tree.get_optional("global.outputs.catchment"); + if (catchment_path_format != boost::none) { + const auto pattern_idx = catchment_path_format->find("{{id}}"); + if (pattern_idx != std::string::npos) { + std::string catchment_path = *catchment_path_format; // copy + return catchment_path.replace(pattern_idx, sizeof("{{id}}") - 1, id); + } + // TODO: same TODO as this::get_nexus_output_path + } + + return "./" + id + ".csv"; + } + protected: std::shared_ptr construct_formulation_from_tree( diff --git a/src/NGen.cpp b/src/NGen.cpp index 5734820960..8c8e179a6b 100644 --- a/src/NGen.cpp +++ b/src/NGen.cpp @@ -316,10 +316,10 @@ int main(int argc, char *argv[]) { for(const auto& id : features.nexuses()) { #ifdef NGEN_MPI_ACTIVE if (!features.is_remote_sender_nexus(id)) { - nexus_outfiles[id].open("./"+id+"_output.csv", std::ios::trunc); + nexus_outfiles[id].open(manager->get_nexus_output_path(id), std::ios::trunc); } #else - nexus_outfiles[id].open("./"+id+"_output.csv", std::ios::trunc); + nexus_outfiles[id].open(manager->get_nexus_output_path(id), std::ios::trunc); #endif } @@ -370,6 +370,7 @@ int main(int argc, char *argv[]) { #ifdef NGEN_MPI_ACTIVE if (!features.is_remote_sender_nexus(id)) { //Ensures only one side of the dual sided remote nexus actually doing this... #endif + //Get the correct "requesting" id for downstream_flow const auto& nexus = features.nexus_at(id); const auto& cat_ids = nexus->get_receiving_catchments(); diff --git a/src/core/HY_Features.cpp b/src/core/HY_Features.cpp index 98fabedeb4..779e0dab01 100644 --- a/src/core/HY_Features.cpp +++ b/src/core/HY_Features.cpp @@ -28,7 +28,7 @@ HY_Features::HY_Features(network::Network network, std::shared_ptrget_formulation(feat_id); - formulation->set_output_stream(feat_id+".csv"); + formulation->set_output_stream(formulations->get_catchment_output_path(feat_id)); // TODO: add command line or config option to have this be omitted //FIXME why isn't default param working here??? get_output_header_line() fails. formulation->write_output("Time Step,""Time,"+formulation->get_output_header_line(",")+"\n"); diff --git a/src/core/HY_Features_MPI.cpp b/src/core/HY_Features_MPI.cpp index 914f1eecb9..315d12716d 100644 --- a/src/core/HY_Features_MPI.cpp +++ b/src/core/HY_Features_MPI.cpp @@ -38,7 +38,7 @@ HY_Features_MPI::HY_Features_MPI( PartitionData partition_data, geojson::GeoJSON { //Find and prepare formulation auto formulation = formulations->get_formulation(feat_id); - formulation->set_output_stream(feat_id+".csv"); + formulation->set_output_stream(formulations->get_catchment_output_path(feat_id)); // TODO: add command line or config option to have this be omitted //FIXME why isn't default param working here??? get_output_header_line() fails. formulation->write_output("Time Step,""Time,"+formulation->get_output_header_line(",")+"\n"); diff --git a/test/realizations/Formulation_Manager_Test.cpp b/test/realizations/Formulation_Manager_Test.cpp index 230125faa1..6780218ffd 100644 --- a/test/realizations/Formulation_Manager_Test.cpp +++ b/test/realizations/Formulation_Manager_Test.cpp @@ -121,6 +121,10 @@ const double EPSILON = 0.0000001; const std::string EXAMPLE_1 = "{ " "\"global\": { " + "\"outputs\": {" + "\"nexus\": \"/tmp/nexus-dir/{{id}}-output.csv\"," + "\"catchment\": \"/tmp/catchment-dir/{{id}}-output.csv\"" + "}," "\"formulations\": [ " "{" "\"name\": \"tshirt\", " @@ -659,6 +663,8 @@ TEST_F(Formulation_Manager_Test, basic_reading_1) { ASSERT_TRUE(manager.contains("cat-52")); ASSERT_TRUE(manager.contains("cat-67")); + ASSERT_EQ(manager.get_nexus_output_path("nex-52"), "/tmp/nexus-dir/nex-52-output.csv"); + ASSERT_EQ(manager.get_catchment_output_path("cat-52"), "/tmp/catchment-dir/cat-52-output.csv"); } TEST_F(Formulation_Manager_Test, basic_reading_2) { From 671e7f5612cabc566feb4a905dbaec14ae60c25d Mon Sep 17 00:00:00 2001 From: program-- Date: Tue, 1 Aug 2023 13:01:11 -0700 Subject: [PATCH 2/3] rev: pullback scope and only support root output directories --- doc/REALIZATION_CONFIGURATION.md | 7 +- .../catchment/Formulation_Manager.hpp | 74 ++++--------------- src/NGen.cpp | 4 +- src/core/HY_Features.cpp | 2 +- src/core/HY_Features_MPI.cpp | 2 +- .../realizations/Formulation_Manager_Test.cpp | 8 +- 6 files changed, 22 insertions(+), 75 deletions(-) diff --git a/doc/REALIZATION_CONFIGURATION.md b/doc/REALIZATION_CONFIGURATION.md index a52e73b983..d41f61333b 100644 --- a/doc/REALIZATION_CONFIGURATION.md +++ b/doc/REALIZATION_CONFIGURATION.md @@ -31,7 +31,7 @@ The `global` key-value object must contain the following two object keys: * `forcing` * key-value object with keys for `file_pattern` and `path` that define the default CSV file pattern and path for the input forcings relative to the executable directory -The `global` key-value object *may* contain an `outputs` object key for user-defined output patterns for nexus and catchment outputs, where the `{{id}}` syntax reflects the nexus or catchment string identifiers (i.e. `cat-27`, `nex-68`). +The `global` key-value object *may* contain an `output_root` key for a user-defined root output directory for nexus and catchment outputs. ``` "global": { @@ -49,10 +49,7 @@ The `global` key-value object *may* contain an `outputs` object key for user-def "file_pattern": ".*{{id}}.*.csv", "path": "./data/forcing/" }, - "outputs": { - "nexus": "/path/to/nexus/{{id}}-output.csv", - "catchment": "/path/to/catchment/{{id}}-output.csv" - } + "output_root": "/path/to/outputs/" }, ``` diff --git a/include/realizations/catchment/Formulation_Manager.hpp b/include/realizations/catchment/Formulation_Manager.hpp index 88279cc8b8..787704060f 100644 --- a/include/realizations/catchment/Formulation_Manager.hpp +++ b/include/realizations/catchment/Formulation_Manager.hpp @@ -251,77 +251,31 @@ namespace realization { } /** - * @brief Get the formatted nexus output path with the given @c id interpolated + * @brief Get the formatted output root * * @code{.cpp} * // Example config: * // ... - * // "outputs": { - * // ... - * // "nexus": "/path/to/nexus-dir/{{id}}-output.csv", - * // ... - * // } + * // "output_root": "/path/to/dir/" * // ... * const auto manager = Formulation_Manger(CONFIG); - * manager.get_nexus_output_path("nex-27"); - * //> "/path/to/nexus-dir/nex-27-output.csv" - * @endcode - * - * @param id Nexus string ID - * @return std::string of the interpolated nexus output path if the realization config file - contains it, otherwise an output path in the form "./{{id}}_output.csv". - */ - std::string get_nexus_output_path(const std::string& id) const noexcept { - const auto nexus_path_format = this->tree.get_optional("global.outputs.nexus"); - if (nexus_path_format != boost::none) { - const auto pattern_idx = nexus_path_format->find("{{id}}"); - if (pattern_idx != std::string::npos) { - std::string nexus_path = *nexus_path_format; // copy - return nexus_path.replace(pattern_idx, sizeof("{{id}}") - 1, id); - } - // TODO: should we still return the path if it doesn't have an ID parameter? - // this would imply that the user specified a path (and may know) that will - // only ever output 1 file. Since, if there is more than 1 output, they will - // overwrite one another. - } - - // TODO: Should catchments and nexi have the same default output format? - return "./" + id + "_output.csv"; - } - - /** - * @brief Get the formatted catchment output path with the given @c id interpolated - * - * @code{.cpp} - * // Example config: - * // ... - * // "outputs": { - * // ... - * // "catchment": "/path/to/catchment-dir/{{id}}-output.csv", - * // ... - * // } - * // ... - * const auto manager = Formulation_Manger(CONFIG); - * manager.get_catchment_output_path("cat-27"); - * //> "/path/to/catchment-dir/cat-27-output.csv" + * manager.get_output_root(); + * //> "/path/to/dir/" * @endcode * - * @param id Catchment string ID - * @return std::string of the interpolated catchment output path if the realization config file - contains it, otherwise an output path in the form "./{{id}}.csv". + * @return std::string of the output root directory */ - std::string get_catchment_output_path(const std::string& id) const noexcept { - const auto catchment_path_format = this->tree.get_optional("global.outputs.catchment"); - if (catchment_path_format != boost::none) { - const auto pattern_idx = catchment_path_format->find("{{id}}"); - if (pattern_idx != std::string::npos) { - std::string catchment_path = *catchment_path_format; // copy - return catchment_path.replace(pattern_idx, sizeof("{{id}}") - 1, id); - } - // TODO: same TODO as this::get_nexus_output_path + std::string get_output_root() const noexcept { + const auto output_root = this->tree.get_optional("global.output_root"); + if (output_root != boost::none && *output_root != "") { + // Check if the path ends with a trailing slash, + // otherwise add it. + return output_root->back() == '/' + ? *output_root + : *output_root + "/"; } - return "./" + id + ".csv"; + return "./"; } diff --git a/src/NGen.cpp b/src/NGen.cpp index 8c8e179a6b..1ef08b05c0 100644 --- a/src/NGen.cpp +++ b/src/NGen.cpp @@ -316,10 +316,10 @@ int main(int argc, char *argv[]) { for(const auto& id : features.nexuses()) { #ifdef NGEN_MPI_ACTIVE if (!features.is_remote_sender_nexus(id)) { - nexus_outfiles[id].open(manager->get_nexus_output_path(id), std::ios::trunc); + nexus_outfiles[id].open(manager->get_output_root() + id + "_output.csv", std::ios::trunc); } #else - nexus_outfiles[id].open(manager->get_nexus_output_path(id), std::ios::trunc); + nexus_outfiles[id].open(manager->get_output_root() + id + "_output.csv", std::ios::trunc); #endif } diff --git a/src/core/HY_Features.cpp b/src/core/HY_Features.cpp index 779e0dab01..69c4543901 100644 --- a/src/core/HY_Features.cpp +++ b/src/core/HY_Features.cpp @@ -28,7 +28,7 @@ HY_Features::HY_Features(network::Network network, std::shared_ptrget_formulation(feat_id); - formulation->set_output_stream(formulations->get_catchment_output_path(feat_id)); + formulation->set_output_stream(formulations->get_output_root() + feat_id + ".csv"); // TODO: add command line or config option to have this be omitted //FIXME why isn't default param working here??? get_output_header_line() fails. formulation->write_output("Time Step,""Time,"+formulation->get_output_header_line(",")+"\n"); diff --git a/src/core/HY_Features_MPI.cpp b/src/core/HY_Features_MPI.cpp index 315d12716d..93609ba91a 100644 --- a/src/core/HY_Features_MPI.cpp +++ b/src/core/HY_Features_MPI.cpp @@ -38,7 +38,7 @@ HY_Features_MPI::HY_Features_MPI( PartitionData partition_data, geojson::GeoJSON { //Find and prepare formulation auto formulation = formulations->get_formulation(feat_id); - formulation->set_output_stream(formulations->get_catchment_output_path(feat_id)); + formulation->set_output_stream(formulations->get_output_root() + feat_id + ".csv"); // TODO: add command line or config option to have this be omitted //FIXME why isn't default param working here??? get_output_header_line() fails. formulation->write_output("Time Step,""Time,"+formulation->get_output_header_line(",")+"\n"); diff --git a/test/realizations/Formulation_Manager_Test.cpp b/test/realizations/Formulation_Manager_Test.cpp index 6780218ffd..b610f4e019 100644 --- a/test/realizations/Formulation_Manager_Test.cpp +++ b/test/realizations/Formulation_Manager_Test.cpp @@ -121,10 +121,7 @@ const double EPSILON = 0.0000001; const std::string EXAMPLE_1 = "{ " "\"global\": { " - "\"outputs\": {" - "\"nexus\": \"/tmp/nexus-dir/{{id}}-output.csv\"," - "\"catchment\": \"/tmp/catchment-dir/{{id}}-output.csv\"" - "}," + "\"output_root\": \"/tmp/output-dir/\"," "\"formulations\": [ " "{" "\"name\": \"tshirt\", " @@ -663,8 +660,7 @@ TEST_F(Formulation_Manager_Test, basic_reading_1) { ASSERT_TRUE(manager.contains("cat-52")); ASSERT_TRUE(manager.contains("cat-67")); - ASSERT_EQ(manager.get_nexus_output_path("nex-52"), "/tmp/nexus-dir/nex-52-output.csv"); - ASSERT_EQ(manager.get_catchment_output_path("cat-52"), "/tmp/catchment-dir/cat-52-output.csv"); + ASSERT_EQ(manager.get_output_root(), "/tmp/output-dir/"); } TEST_F(Formulation_Manager_Test, basic_reading_2) { From 325aa4b986822cb73f3d49dec9d5be4d045b7d71 Mon Sep 17 00:00:00 2001 From: program-- Date: Thu, 3 Aug 2023 07:34:21 -0700 Subject: [PATCH 3/3] rev: move `output_root` outside global key Changes per https://github.com/NOAA-OWP/ngen/pull/531#issuecomment-1664005100 --- doc/REALIZATION_CONFIGURATION.md | 10 +++++----- include/realizations/catchment/Formulation_Manager.hpp | 2 +- test/realizations/Formulation_Manager_Test.cpp | 3 ++- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/doc/REALIZATION_CONFIGURATION.md b/doc/REALIZATION_CONFIGURATION.md index d41f61333b..2635b91ab3 100644 --- a/doc/REALIZATION_CONFIGURATION.md +++ b/doc/REALIZATION_CONFIGURATION.md @@ -15,11 +15,14 @@ The Configuration is a key-value object and must contain these three first level * `catchments` * is a key-value object that must include a list of individual catchments +The configuration may *optionally* contain an `output_root` key with a user-defined root output directory as the key, for nexus and catchment outputs. + ``` { "global": {}, "time": {}, - "catchments": {} + "catchments": {}, + "output_root": "/path/to/output/" } ``` @@ -31,8 +34,6 @@ The `global` key-value object must contain the following two object keys: * `forcing` * key-value object with keys for `file_pattern` and `path` that define the default CSV file pattern and path for the input forcings relative to the executable directory -The `global` key-value object *may* contain an `output_root` key for a user-defined root output directory for nexus and catchment outputs. - ``` "global": { "formulations": [ @@ -48,8 +49,7 @@ The `global` key-value object *may* contain an `output_root` key for a user-defi "forcing": { "file_pattern": ".*{{id}}.*.csv", "path": "./data/forcing/" - }, - "output_root": "/path/to/outputs/" + } }, ``` diff --git a/include/realizations/catchment/Formulation_Manager.hpp b/include/realizations/catchment/Formulation_Manager.hpp index 787704060f..31f957c5a3 100644 --- a/include/realizations/catchment/Formulation_Manager.hpp +++ b/include/realizations/catchment/Formulation_Manager.hpp @@ -266,7 +266,7 @@ namespace realization { * @return std::string of the output root directory */ std::string get_output_root() const noexcept { - const auto output_root = this->tree.get_optional("global.output_root"); + const auto output_root = this->tree.get_optional("output_root"); if (output_root != boost::none && *output_root != "") { // Check if the path ends with a trailing slash, // otherwise add it. diff --git a/test/realizations/Formulation_Manager_Test.cpp b/test/realizations/Formulation_Manager_Test.cpp index b610f4e019..531f5d8af2 100644 --- a/test/realizations/Formulation_Manager_Test.cpp +++ b/test/realizations/Formulation_Manager_Test.cpp @@ -120,8 +120,8 @@ class Formulation_Manager_Test : public ::testing::Test { const double EPSILON = 0.0000001; const std::string EXAMPLE_1 = "{ " + "\"output_root\": \"/tmp/output-dir/\"," "\"global\": { " - "\"output_root\": \"/tmp/output-dir/\"," "\"formulations\": [ " "{" "\"name\": \"tshirt\", " @@ -683,6 +683,7 @@ TEST_F(Formulation_Manager_Test, basic_reading_2) { ASSERT_TRUE(manager.contains("cat-52")); ASSERT_TRUE(manager.contains("cat-67")); + ASSERT_EQ(manager.get_output_root(), "./"); } TEST_F(Formulation_Manager_Test, basic_run_1) {