-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify Formulation_Manager.hpp to parse forcing "file_pattern" #489
Conversation
This looks promising... I will try testing with the files tomorrow... could you try adding an automated test for this? Would probably make sense in |
Yes, sure.
…On Thu, Jan 12, 2023 at 4:04 PM Matt Williamson ***@***.***> wrote:
This looks promising... I will try testing with the files tomorrow...
could you try adding an automated test for this? Would probably make sense
in Formulation_Manager_Test.cpp.
—
Reply to this email directly, view it on GitHub
<#489 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRM7O3E5SSGYMF6OFM3WSB5YFANCNFSM6AAAAAATZUOZ3E>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me. Doesn't change the existing semantics but allows significant code reuse! My only concern is the test coverage now doesn't consider the case when an explicit path is passed.
@@ -608,7 +620,9 @@ const std::string EXAMPLE_4 = "{ " | |||
"} " | |||
"], " | |||
"\"forcing\": { " | |||
"\"path\": \"./data/forcing/cat-67_2015-12-01 00_00_00_2015-12-30 23_00_00.csv\" " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave one or two of these tests with just the path
and not pattern
to ensure that both cases work.
If there is no disagreement, I'll implement Nels' suggestion.
…On Tue, Jan 17, 2023 at 10:48 AM Nels ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Generally looks good to me. Doesn't change the existing semantics but
allows significant code reuse! My only concern is the test coverage now
doesn't consider the case when an explicit path is passed.
------------------------------
In test/realizations/Formulation_Manager_Test.cpp
<#489 (comment)>:
> @@ -608,7 +620,9 @@ const std::string EXAMPLE_4 = "{ "
"} "
"], "
"\"forcing\": { "
- "\"path\": \"./data/forcing/cat-67_2015-12-01 00_00_00_2015-12-30 23_00_00.csv\" "
I would leave one or two of these tests with just the path and not pattern
to ensure that both cases work.
—
Reply to this email directly, view it on GitHub
<#489 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRITFQ3WCVN3Z3XAKFDWS3ENTANCNFSM6AAAAAATZUOZ3E>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This commit enable ngen to parse forcing "file_pattern" in explicit catchments formulation.
Additions
data/test_bmi_multi_realization_config.json
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 tests
run ngen tests in serial
run ngen tests in parallel
Screenshots
Notes
Todos
Checklist
Testing checklist (automated report can be put here)
Target Environment support