Gridded output formulations #532

stcui007 · 2023-05-26T20:19:02Z

Output variables on selected grid points

Additions

Removals

Changes

gridded_bmi_aorc_python/config.yml
include/realizations/catchment/Bmi_C_Formulation.hpp
include/realizations/catchment/Bmi_Cpp_Formulation.hpp
include/realizations/catchment/Bmi_Fortran_Formulation.hpp
include/realizations/catchment/Bmi_Module_Formulation.hpp
include/realizations/catchment/Bmi_Multi_Formulation.hpp
include/realizations/catchment/Bmi_Py_Adapter.hpp
include/realizations/catchment/Bmi_Py_Formulation.hpp
src/realizations/catchment/Bmi_C_Formulation.cpp
src/realizations/catchment/Bmi_Cpp_Formulation.cpp
src/realizations/catchment/Bmi_Fortran_Formulation.cpp
src/realizations/catchment/Bmi_Multi_Formulation.cpp
src/realizations/catchment/Bmi_Py_Formulation.cpp
test/realizations/catchments/Bmi_Multi_Formulation_Test.cpp
test/realizations/catchments/Bmi_Py_Formulation_Test.cpp

Testing

Unit Tests
Run ngen test

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
[x ] 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

Review is incomplete but there are some things to start on here.

In addition to the inline comments, we need all the unrelated content removed from the PR to consider merging (e.g. the AORC modules)--there's no need to have data sources for gridded data beyond the existing test models (I don't think), though yes we will need some tests for this the data can probably be either provided by the test models or hard-coded into the unit tests (I haven't reviewed the test code yet).

mattw-nws · 2023-06-05T13:27:23Z

include/realizations/catchment/Bmi_Multi_Formulation.hpp

+                long nested_module_time = nested_module->get_data_start_time() + ( this->get_model_current_time() - this->get_model_start_time() );
+                auto selector = CatchmentAggrDataSelector(this->get_catchment_id(),var_name,nested_module_time,this->record_duration(),"1");
+                //TODO: After merge PR#405, try re-adding support for index
+                return nested_module->get_values(selector);


So, this resulted in a copy of the function get_var_value_as_double with exactly one character changed (aside from the signature). Since this is a moderately complex function, this duplication is not good. Once this function is implemented, get_var_value_as_double is a special case of get_var_vec_as_double, which is to get a specific index (default 0). Also note the //TODO: above. All of this to say the following:

When adding this function, the function get_var_value_as_double should probably call get_var_vec_as_double instead of duplicating its contents. Also, this function (get_var_vec_as_double) does not use the index parameter, it should be removed from the signature here. However, get_var_value_as_double used to use the index parameter but it was removed in #405; and if get_var_value_as_double calls get_var_vec_as_double, it can probably easily re-implement the index parameter now, so this should probably be done.

The above change (and re-implementation of the index parameter) probably makes sense to do in the other Bmi_*_Formulation classes as well, though their get_var_vec_as_double methods are probably much less complex.

Another approach to remove the duplication might be to implement get_data_provider_for_var_name, which would return a pointer to a data provider (which is what most of this function does), but even if doing that, get_var_value_as_double should probably be made to call get_var_vec_as_double in order to implement the index parameter... so this decomposition could easily be accomplished later.

Another note here, it is probably possible to get get_var_value_as_double to return const & to avoid additional copies, but I'm not 100% sure...but think about avoiding additional copies in this PR where possible.

Regarding the first comment, I agree that it does seem out of place for that AORC code to be there. I'll remove it from this PR. On the other hand, the code is needed at present to be able to run the gridded python module. Maybe we can put that code in ngen-forcing repo and download it whenever we want to run the Bmi-Python module in ngen?

The comments about the get_var_vec_as_double are well taken. I'll simplify the implementation along the line suggested in the comment.

mattw-nws · 2023-06-05T13:29:14Z

include/realizations/catchment/Bmi_Py_Formulation.hpp

@@ -97,12 +97,20 @@ namespace realization {

        shared_ptr<models::bmi::Bmi_Py_Adapter> construct_model(const geojson::PropertyMap &properties) override;

+        template<class T, class O>
+        T get_var_value_as(int t_index, const std::string& var_name) {


This function is declared in several classes but never used... please remove it from this PR if it isn't part of the solution.

Yeah, it was an earlier implementation but not get used in the final version. One version of the template function itself probably is in the master repo. I'll check.

mattw-nws · 2023-06-05T14:08:21Z

src/realizations/catchment/Bmi_Cpp_Formulation.cpp

    for (const std::string& name : get_output_variable_names()) {
-        output_str += (output_str.empty() ? "" : ",") + std::to_string(get_var_value_as_double(name));
+        //output_str += (output_str.empty() ? "" : ",") + std::to_string(get_var_value_as_double(name));
+        output_str += (output_str.empty() ? "" : ",") + std::to_string(get_var_vec_as_double(t_delta, name)[0]);


A simple solution for scalar values would be to check the vector length and if it's = 1 then output only element 0 .... I think that get_var_vec_as_double should work for scalar values returning a vector of size 1. But yes, this should be addressed before we consider merging.

I'm also not sure we actually want to put this to_string representation into a column in the CSV... what does it look like? If it has commas that won't do at all, just for instance.

Here is an example output:
cat-298118.csv
or as follows from direct copy/paste. In the displayed data, each variable has 9 consecutive values. I think the comas within each variable can be easily removed if that's what we want.

TimeStep,Time,APCP_surface,TMP_2maboveground,SPFH_2maboveground,UGRD_10maboveground,VGRD_10maboveground,PRES_surface,DSWRF_surface,DLWRF_surface 0,2015-12-01 00:00:00,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,278.399993896,278.600006104,278.600006104,278.399993896,278.500000000,278.600006104,278.399993896,278.399993896,-0.123450004,0.004700000,0.004800000,0.004800000,0.004700000,0.004700000,0.004800000,0.004700000,0.004700000,-0.123450004,5.700000286,5.700000286,5.700000286,5.700000286,5.700000286,5.700000286,5.700000286,5.700000286,-0.123450004,1.500000000,1.500000000,1.600000024,1.500000000,1.500000000,1.500000000,1.500000000,1.500000000,-0.123450004,98070.000000000,98020.000000000,97970.000000000,98130.000000000,98070.000000000,98020.000000000,98080.000000000,98050.000000000,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,-0.123450004,247.500000000,248.100006104,248.300003052,247.300003052,247.600006104,248.100006104,247.100006104,247.199996948,-0.123450004 1,2015-12-01 01:00:00,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,276.899993896,276.899993896,277.000000000,276.899993896,276.899993896,277.000000000,276.899993896,276.899993896,-0.123450004,0.004400000,0.004400000,0.004500000,0.004400000,0.004400000,0.004400000,0.004400000,0.004400000,-0.123450004,4.900000095,4.900000095,4.900000095,4.900000095,4.900000095,4.900000095,4.900000095,4.900000095,-0.123450004,0.699999988,0.699999988,0.800000012,0.699999988,0.699999988,0.699999988,0.699999988,0.699999988,-0.123450004,98020.000000000,97970.000000000,97920.000000000,98080.000000000,98000.000000000,97970.000000000,98040.000000000,98000.000000000,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,-0.123450004,248.600006104,248.600006104,249.100006104,248.300003052,248.500000000,248.800003052,248.100006104,248.100006104,-0.123450004 2,2015-12-01 02:00:00,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,275.500000000,275.500000000,275.500000000,275.399993896,275.500000000,275.500000000,275.399993896,275.399993896,-0.123450004,0.004200000,0.004200000,0.004200000,0.004100000,0.004200000,0.004200000,0.004100000,0.004100000,-0.123450004,4.099999905,4.099999905,4.099999905,4.099999905,4.099999905,4.099999905,4.099999905,4.099999905,-0.123450004,-0.100000001,-0.100000001,-0.100000001,-0.100000001,-0.100000001,-0.100000001,-0.100000001,-0.100000001,-0.123450004,97960.000000000,97910.000000000,97860.000000000,98020.000000000,97960.000000000,97910.000000000,97980.000000000,97940.000000000,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,-0.123450004,249.900009155,249.900009155,250.300003052,249.699996948,249.800003052,250.000000000,249.400009155,249.500000000,-0.123450004 3,2015-12-01 03:00:00,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,274.100006104,274.000000000,274.100006104,274.000000000,274.000000000,274.100006104,274.000000000,274.000000000,-0.123450004,0.003900000,0.003900000,0.003900000,0.003900000,0.003900000,0.003900000,0.003900000,0.003900000,-0.123450004,3.299999952,3.299999952,3.400000095,3.299999952,3.299999952,3.299999952,3.299999952,3.299999952,-0.123450004,-0.900000036,-0.900000036,-0.900000036,-0.900000036,-0.900000036,-0.900000036,-0.900000036,-0.900000036,-0.123450004,97910.000000000,97870.000000000,97800.000000000,97960.000000000,97900.000000000,97850.000000000,97910.000000000,97880.000000000,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,-0.123450004,219.000000000,218.800003052,219.000000000,218.800003052,218.800003052,218.800003052,219.000000000,218.800003052,-0.123450004 4,2015-12-01 04:00:00,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,273.399993896,273.300018311,273.399993896,273.399993896,273.399993896,273.399993896,273.399993896,273.300018311,-0.123450004,0.003700000,0.003700000,0.003700000,0.003700000,0.003700000,0.003700000,0.003700000,0.003700000,-0.123450004,3.200000048,3.200000048,3.200000048,3.200000048,3.200000048,3.200000048,3.200000048,3.200000048,-0.123450004,-0.600000024,-0.600000024,-0.600000024,-0.600000024,-0.600000024,-0.600000024,-0.600000024,-0.600000024,-0.123450004,97910.000000000,97870.000000000,97800.000000000,97960.000000000,97900.000000000,97850.000000000,97910.000000000,97880.000000000,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,-0.123450004,219.400009155,219.100006104,219.300003052,219.300003052,219.300003052,219.300003052,219.300003052,219.100006104,-0.123450004 5,2015-12-01 05:00:00,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,272.700012207,272.600006104,272.700012207,272.700012207,272.700012207,272.600006104,272.700012207,272.600006104,-0.123450004,0.003600000,0.003600000,0.003600000,0.003600000,0.003600000,0.003600000,0.003600000,0.003600000,-0.123450004,3.000000000,3.000000000,3.100000143,3.000000000,3.000000000,3.000000000,3.000000000,3.000000000,-0.123450004,-0.300000012,-0.300000012,-0.300000012,-0.300000012,-0.300000012,-0.300000012,-0.200000003,-0.200000003,-0.123450004,97910.000000000,97870.000000000,97800.000000000,97980.000000000,97900.000000000,97850.000000000,97930.000000000,97880.000000000,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,-0.123450004,219.800003052,219.500000000,219.699996948,219.699996948,219.699996948,219.699996948,219.800003052,219.500000000,-0.123450004 6,2015-12-01 06:00:00,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,272.000000000,271.899993896,271.899993896,272.000000000,272.000000000,271.899993896,272.000000000,272.000000000,-0.123450004,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,-0.123450004,2.900000095,2.900000095,2.900000095,2.900000095,2.900000095,2.900000095,2.900000095,2.900000095,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.100000001,0.100000001,-0.123450004,97910.000000000,97870.000000000,97800.000000000,97980.000000000,97900.000000000,97870.000000000,97930.000000000,97900.000000000,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,-0.123450004,215.600006104,215.199996948,215.199996948,215.500000000,215.400009155,215.199996948,215.600006104,215.400009155,-0.123450004 7,2015-12-01 07:00:00,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,271.899993896,271.800018311,271.800018311,271.899993896,271.899993896,271.800018311,271.899993896,271.899993896,-0.123450004,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,-0.123450004,2.500000000,2.500000000,2.500000000,2.500000000,2.500000000,2.500000000,2.500000000,2.500000000,-0.123450004,0.900000036,0.900000036,0.900000036,0.900000036,0.900000036,0.900000036,0.900000036,0.900000036,-0.123450004,97930.000000000,97880.000000000,97830.000000000,97990.000000000,97930.000000000,97880.000000000,97950.000000000,97910.000000000,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,-0.123450004,215.500000000,215.300003052,215.300003052,215.400009155,215.400009155,215.300003052,215.500000000,215.300003052,-0.123450004 8,2015-12-01 08:00:00,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,271.800018311,271.700012207,271.800018311,271.800018311,271.800018311,271.800018311,271.800018311,271.800018311,-0.123450004,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,0.003400000,-0.123450004,2.100000143,2.100000143,2.100000143,2.100000143,2.100000143,2.100000143,2.100000143,2.100000143,-0.123450004,1.700000048,1.700000048,1.700000048,1.700000048,1.700000048,1.700000048,1.700000048,1.700000048,-0.123450004,97950.000000000,97900.000000000,97850.000000000,98010.000000000,97950.000000000,97900.000000000,97960.000000000,97930.000000000,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,-0.123450004,215.400009155,215.100006104,215.100006104,215.400009155,215.300003052,215.100006104,215.500000000,215.300003052,-0.123450004 9,2015-12-01 09:00:00,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,271.700012207,271.600006104,271.700012207,271.700012207,271.700012207,271.700012207,271.700012207,271.700012207,-0.123450004,0.003400000,0.003300000,0.003400000,0.003400000,0.003400000,0.003300000,0.003400000,0.003400000,-0.123450004,1.800000072,1.800000072,1.800000072,1.800000072,1.800000072,1.800000072,1.800000072,1.800000072,-0.123450004,2.600000143,2.600000143,2.600000143,2.600000143,2.600000143,2.600000143,2.600000143,2.600000143,-0.123450004,97960.000000000,97930.000000000,97870.000000000,98030.000000000,97960.000000000,97910.000000000,97980.000000000,97950.000000000,-0.123450004,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,0.000000000,-0.123450004,213.199996948,212.800003052,212.900009155,213.100006104,212.900009155,212.800003052,213.100006104,212.900009155,-0.123450004

On scalar vs. vector, the get_output_line_for_timestep() function should be made to be able to output scalar and vector values too.

mattw-nws · 2023-06-05T17:17:15Z

src/realizations/catchment/Bmi_Fortran_Formulation.cpp

    // Do the first separately, without the leading comma
-    *output_text_stream << get_var_value_as_double(output_var_names[0]);
+    //*output_text_stream << "," << get_var_value_as_double(output_var_names[0]);
+    *output_text_stream << get_var_vec_as_double(t_delta, output_var_names[0])[0];


Comments on [Bmi_Cpp_Formulation.cpp] apply to all get_output_line_for_timestep implementations... unifying the code in these would also be beneficial (and in scope since they all have to be touched).... consider, is there a way to make this a superclass method or share some code between these?

Just noticed this comment before I wrote mine. Agree.

mattw-nws · 2023-06-05T17:24:45Z

src/realizations/catchment/Bmi_Py_Formulation.cpp

+    //*output_text_stream <<"," << get_var_vec_as_double(t_delta, output_var_names[0])[17646500];
+
+    // Using array to select indices of grid points for output data
+    int array[5] = {0,1,17646300,17646500,17646000};


Remove test data... lets keep subsetting of output out of this PR.

OK. It is a validation that vectorized/gridded version works.

mattw-nws

Review is incomplete but there are some things to start on here.

In addition to the inline comments, we need all the unrelated content removed from the PR to consider merging (e.g. the AORC modules)--there's no need to have data sources for gridded data beyond the existing test models (I don't think), though yes we will need some tests for this the data can probably be either provided by the test models or hard-coded into the unit tests (I haven't reviewed the test code yet).

stcui007 · 2023-06-06T14:13:39Z

I have replied to your comments in the PR page. Regarding to the template function T get_var_value_as(time_step_t t_index, const std::string& var_name), the codes exist in the Bmi_C_Formulation.hpp and Bmi_Cpp_Formulation.hpp in the master repo, do you want to leave them in there as they are?

…

On Mon, Jun 5, 2023 at 12:59 PM Matt Williamson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/realizations/catchment/Bmi_Multi_Formulation.hpp <#532 (comment)>: > + * Get value vector for some BMI model variable. + */ + std::vector<double> get_var_vec_as_double(const int& index, const std::string& var_name) { + auto data_provider_iter = availableData.find(var_name); + if (data_provider_iter == availableData.end()) { + throw external::ExternalIntegrationException( + "Multi BMI formulation can't find correct nested module for BMI variable " + var_name + SOURCE_LOC); + } + // Otherwise, we have a provider, and we can cast it based on the documented assumptions + try { + std::shared_ptr <data_access::GenericDataProvider> nested_module = + std::dynamic_pointer_cast<data_access::GenericDataProvider>(data_provider_iter->second); + long nested_module_time = nested_module->get_data_start_time() + ( this->get_model_current_time() - this->get_model_start_time() ); + auto selector = CatchmentAggrDataSelector(this->get_catchment_id(),var_name,nested_module_time,this->record_duration(),"1"); + //TODO: After merge PR#405, try re-adding support for index + return nested_module->get_values(selector); Another note here, it is probably possible to get get_var_value_as_double to return const & to avoid additional copies, but I'm not 100% sure...but think about avoiding additional copies in this PR where possible. — Reply to this email directly, view it on GitHub <#532 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACA4SRO5WTCWPA7Y3BXW2A3XJYM7HANCNFSM6AAAAAAYQU4GVU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

mattw-nws · 2023-06-30T20:19:27Z

Superseded by #534

stcui007 added 4 commits May 9, 2023 20:15

Gridded AORC BMI model with Jinjabmi

88df25c

Add README.md, etc.

655e84c

Use template get_var_value_as() function for gridded output

4779429

Output variables on selected grid points

3afd05e

stcui007 requested review from hellkite500, robertbartel and mattw-nws May 26, 2023 20:19

mattw-nws requested changes Jun 5, 2023

View reviewed changes

stcui007 added 4 commits June 8, 2023 20:19

Deleting gridded_bmi_aorc_python directory

e351b81

Revised gridded output formulation addressing comments

a9e4d82

Fixing unit test error.

e06cef0

Fixing BMI fortran formulation unit test

2da8555

hellkite500 mentioned this pull request Jun 27, 2023

Array Broadcasting Semantics for BMI Formulations #551

Open

mattw-nws closed this Jun 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gridded output formulations #532

Gridded output formulations #532

stcui007 commented May 26, 2023

mattw-nws left a comment

mattw-nws Jun 5, 2023

mattw-nws Jun 5, 2023

stcui007 Jun 5, 2023

mattw-nws Jun 5, 2023

stcui007 Jun 5, 2023

mattw-nws Jun 5, 2023

stcui007 Jun 5, 2023

stcui007 Jun 5, 2023

mattw-nws Jun 5, 2023

stcui007 Jun 5, 2023

mattw-nws Jun 5, 2023

stcui007 Jun 5, 2023

mattw-nws left a comment

stcui007 commented Jun 6, 2023 via email

mattw-nws commented Jun 30, 2023

Gridded output formulations #532

Gridded output formulations #532

Conversation

stcui007 commented May 26, 2023

Additions

Removals

Changes

Testing

Screenshots

Notes

Todos

Checklist

Testing checklist (automated report can be put here)

Target Environment support

mattw-nws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattw-nws left a comment

Choose a reason for hiding this comment

stcui007 commented Jun 6, 2023 via email

mattw-nws commented Jun 30, 2023