-
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
Updated realization configs in data/
#788
base: master
Are you sure you want to change the base?
Conversation
I set this as draft because I have a couple questions:
|
The following list current cfe module input variables:
https://github.com/NOAA-OWP/cfe/blob/master/src/bmi_cfe.c#L278-L283
…On Thu, Apr 4, 2024 at 7:33 AM K. Jennings ***@***.***> wrote:
I set this as draft because I have a couple questions:
- @ajkhattak <https://github.com/ajkhattak> Can you confirm the
mapping for CFE variables? Specifically, I'm not certain if CFE requires
SLoTH output to run in the framework. For example, you'll see SLoTH
variables mapped in example_bmi_multi_realization_config.json here
<https://github.com/NOAA-OWP/ngen/blob/694a3c7d330af63e84fb3a2dd6723cb0b5148c20/data/example_bmi_multi_realization_config.json#L61>
but not in example_bmi_multi_realization_config__macos.json here
<https://github.com/NOAA-OWP/ngen/blob/694a3c7d330af63e84fb3a2dd6723cb0b5148c20/data/example_bmi_multi_realization_config__macos.json#L45>
.
- @hellkite500 <https://github.com/hellkite500> Is the
test_realization_config.json config used anymore? I noticed it has
t-shirt parameters, which seems out of date.
—
Reply to this email directly, view it on GitHub
<#788 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRP7O4WAXJOVAXNIXZDY3VCBLAVCNFSM6AAAAABFXFRZGKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZXGA3DOMBZGU>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Thanks for that info, @stcui007. I could've been clearer in my question for @ajkhattak. CFE will set those values if running uncouple (example here), so I was hoping Ahmad could check if my assumptions were correct. |
@ajkhattak <https://github.com/ajkhattak> is the right person to answer
that question.
…On Thu, Apr 4, 2024 at 8:45 AM K. Jennings ***@***.***> wrote:
Thanks for that info, @stcui007 <https://github.com/stcui007>. I could've
been clearer in my question for @ajkhattak <https://github.com/ajkhattak>.
CFE will set those values if running uncouple (example here
<https://github.com/NOAA-OWP/cfe/blob/6d08db7bec2ec6cc2cc2446300c9e0d2076e0fc7/src/cfe.c#L125>),
so I was hoping Ahmad could check if my assumptions were correct.
—
Reply to this email directly, view it on GitHub
<#788 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACA4SRPEPWSHDPFTMEFJTCDY3VKQJAVCNFSM6AAAAABFXFRZGKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZXGI3DOOBYGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
CFE does require SLoTh outputs if running in the framework in standalone mode (i.e., not coupled with SMP (soil moisture profiles) and SFT (soil freeze-thaw)) this example is outdated. Just a note: NONE of the soil models (other than the topmodel) will run in the framework without SLoTH outputs. |
@SnowHydrology One clarification here, CFE takes
ensure that |
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.
This file is missing SLoTH block, but I think this is the same example as data/example_bmi_multi_realization_config.json and should be removed.
In the past, ngen had to keep copies of realization files for Linux and macOS machines, so files ending with __macos.json
should be deleted, as we no longer provide .dylib
or .so
extensions. (@hellkite500 is my understanding correct?)
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.
@hellkite500 Can you confirm?
@@ -60,16 +60,10 @@ | |||
"registration_function": "register_bmi_cfe", | |||
"variables_names_map": { | |||
"water_potential_evaporation_flux": "ETRAN", |
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.
Here it is
"water_potential_evaporation_flux": "ETRAN",
however, in example_bmi_multi_realization_config_w_netcdf.json
, it is
"water_potential_evaporation_flux": "EVAPOTRANS",
it should be EVAPOTRANS
as well.
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.
Good catch. Fixed.
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.
Added a few comments, other than that most of these files look good to me.
@@ -68,13 +68,7 @@ | |||
"registration_function": "register_bmi_cfe", | |||
"variables_names_map": { | |||
"water_potential_evaporation_flux": "ETRAN", |
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.
ETRAN -> EVAPOTRANS??
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.
Fixed
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.
also, I think these .so
extensions can be removed from the library files. @hellkite500
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.
@hellkite500, please confirm
I have done tests running |
Thanks @stcui007! I think we're just waiting on a quick look from @hellkite500 to determine which configs are still used and which can be deleted. |
Since this one has been there for a long time, I decided to take a look at it together with PR#775 to set them up to merge. To @hellkite500's question about those The In realizations involve both PET and CFE, depending on preference, the following line of mapping is not absolutely necessary, I think.
|
#787 indicates that the example realization configs in the
data
directory are out of date. Notably, none of the realizations mapped Noah-OWP-ModularQINSUR
output to CFE, meaning the latter was taking precipitation directly from forcing.Additions
Removals
Changes
Updated the
variables_names_map
in:example_bmi_multi_realization_config.json
example_bmi_multi_realization_config__macos.json
example_bmi_multi_realization_config_w_netcdf.json
example_bmi_multi_realization_config_w_nfp.json
example_bmi_multi_realization_config_w_noah_pet_cfe.json
example_bmi_multi_realization_config_w_routing.json
test_bmi_multi_realization_config.json
test_bmi_multi_realization_config_w_netcdf.json
test_bmi_multi_realization_config_w_noah_pet_cfe.json
Testing
Screenshots
Notes
Todos
Checklist
Testing checklist (automated report can be put here)
Target Environment support