Fix a bug in output variable name list, replacing (PR#577) #610
Conversation
There was a problem hiding this comment.
This is indeed cleaner, but it goes back to only fixing the problem for Bmi_Multi_Formulation where the problem in #535 can occur in any BMI formulation. This should be possible to do up in Bmi_Formulation or at least with only a little duplicate code in Bmi_Multi_Formulation and Bmi_Module_Formulation
| // take into account the unit test where both the original and remapped output var names are used in the test codes | ||
| for (const nested_module_ptr &module: modules) { | ||
| for (const std::string &out_var_name: module->get_bmi_output_variables()) { | ||
| if ((out_var_name == "OUTPUT_VAR_1") || (out_var_name == "OUTPUT_VAR_2") || (out_var_name == "OUTPUT_VAR_3")) { |
There was a problem hiding this comment.
I'm not sure what's going on in the unit test (I'll try to take a closer look today), but putting these "magic strings" in the production code to account for behavior in the unit test isn't the right answer. Either the test is wrong, or the behavior needs to match the test.
There was a problem hiding this comment.
This is indeed cleaner, but it goes back to only fixing the problem for Bmi_Multi_Formulation where the problem in #535 can occur in any BMI formulation. This should be possible to do up in Bmi_Formulation or at least with only a little duplicate code in Bmi_Multi_Formulation and Bmi_Module_Formulation
|
This is the only place where all the data are accessible. The availableData
variable actually contains all variables in the sub-modules as well as
input forcing, so nothing is left out. If you run test on the example
realization files you provided, they all work as they are supposed to.
Somehow, in Bmi_Formulation or Bmi_Module_formulation, not all data are
accessible at the same time, so a validity comparison could not be made.
On the unit test part, I agree that it does look a bit awkward. But the
unit test code uses both original and remapped output variables in the
codes. Since the original output variables is already remapped, it does not
exist anymore, so the ngen code generate a "throw" by design. It seems to
be trapped.
…On Tue, Aug 15, 2023 at 10:59 AM Matt Williamson ***@***.***> wrote:
***@***.**** requested changes on this pull request.
This is indeed cleaner, but it goes back to only fixing the problem for
Bmi_Multi_Formulation where the problem in #535
<#535> can occur in any BMI
formulation. This should be possible to do up in Bmi_Formulation or at
least with only a little duplicate code in Bmi_Multi_Formulation and
Bmi_Module_Formulation
—
Reply to this email directly, view it on GitHub
<#610 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRNTXG32LGOBEMLJ4VTXVOMFJANCNFSM6AAAAAA3RD7PJ4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
| @@ -292,6 +292,11 @@ string Bmi_Multi_Formulation::get_output_line_for_timestep(int timestep, std::st | |||
| throw std::invalid_argument("Only current time step valid when getting multi-module BMI formulation output"); | |||
| } | |||
|
|
|||
| // check if a requested output variable name is valid, if not, stop the execution | |||
| if (timestep == 0) { | |||
| check_output_var_names(); | |||
There was a problem hiding this comment.
I think this will work if this function is called at the end of create_multi_formulation instead of in the time loop... can you try it there instead?
|
Sure will do.
…On Thu, Aug 17, 2023 at 8:24 AM Matt Williamson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/realizations/catchment/Bmi_Multi_Formulation.cpp
<#610 (comment)>:
> @@ -292,6 +292,11 @@ string Bmi_Multi_Formulation::get_output_line_for_timestep(int timestep, std::st
throw std::invalid_argument("Only current time step valid when getting multi-module BMI formulation output");
}
+ // check if a requested output variable name is valid, if not, stop the execution
+ if (timestep == 0) {
+ check_output_var_names();
I think this will work if this function is called at the end of
create_multi_formulation instead of in the time loop... can you try it
there instead?
—
Reply to this email directly, view it on GitHub
<#610 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRPKE4KIE6TDJFXTHS3XVYLRXANCNFSM6AAAAAA3RD7PJ4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This PR fixing variable name entry issue#535
Additions
Removals
Changes
include/realizations/catchment/Bmi_Multi_Formulation.hpp
src/realizations/catchment/Bmi_Multi_Formulation.cpp
Testing
Unit Tests
Run ngen tests
Screenshots
Notes
Todos
Checklist
Testing checklist (automated report can be put here)
Target Environment support