Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Testing and Documenting Building, Running, Forcing, Hydrofabric, Initial Input Data and Realization Config on CONUS #775

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Testing and Documenting Building, Running, Forcing, Hydrofabric, Initial Input Data and Realization Config on CONUS #775

wants to merge 12 commits into from

Conversation

stcui007
Copy link
Contributor

@stcui007 stcui007 commented Mar 22, 2024 •
edited
Loading

This is an initial set of realization configs with successive addition of submodules.

Additions

Realization configs for sloth, sloth+noah, sloth+noah+pet, sloth+noah+pet+cfe;
sloth+noah+pet+spm, sloth+noah+pet+smp+sft, sloth+noah+pet+smp+sft+cfe;
some configs with routing, and two configs with laternative module ordering which may provide better performance according to hydrologist.

Removals

Changes

Testing

  1. Run ngen testing on local servers

Screenshots

Notes

More additions upcoming

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)
  • [x ] 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)
  • [x ] Reviewers requested with the Reviewers tool ➡️

Testing checklist (automated report can be put here)

Target Environment support

  • [x ] Linux

@hellkite500
Copy link
Member

Per some slack discussions, some things we need to consider and probably modify on these configurations

from @SnowHydrology

I wouldn’t expect QINSUR to be successfully passed from NOM to CFE unless the variable mapping is set correctly, which it doesn’t appear to be. It may be that you’re running CFE, but getting the precipitation directly from the forcing instead of as output from NOM. (edited)

Also, why are the met forcings mapped in bmi_c_cfe? CFE doesn’t use them

SnowHydrology
SnowHydrology requested changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@SnowHydrology SnowHydrology left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start and I've made a few suggestions of items that need updating. Please note that I call each issue out one time, but many of the issues occur multiple times. For example, I note the missing PET forcing variable mapping in bmi_multi_realization_config_w_sloth_pet_cfe_nc.json, but it's also found in the other configs that use PET. Please make the suggested changes across all relevant files, not just the one file where the issue is noted. Thanks!

data/baseline/bmi_multi_realization_config_w_sloth_pet_cfe_nc.json Outdated
"main_output_variable": "water_potential_evaporation_flux",
"registration_function": "register_bmi_pet",
"variables_names_map": {
"water_potential_evaporation_flux": "potential_evapotranspiration"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PET is missing the mapping for the other forcing variables that are found in the CFE variable_names_map below. Specifically, this includes:

 "atmosphere_air_water~vapor__relative_saturation" : "SPFH_2maboveground",
"land_surface_air__temperature" : "TMP_2maboveground",
"land_surface_wind__x_component_of_velocity" : "UGRD_10maboveground",
"land_surface_wind__y_component_of_velocity" : "VGRD_10maboveground",
"land_surface_radiation~incoming~longwave__energy_flux" : "DLWRF_surface",
"land_surface_radiation~incoming~shortwave__energy_flux" : "DSWRF_surface",
"land_surface_air__pressure" : "PRES_surface"

I also don't think you need the mapping of the output variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The realization configs have taken a major overhaul largely following @SnowHydrology's PR#788 for relevant configs. So the above point is addressed in these and the ones involve soil submodules. The above point is correct and CFE only has 5 input variables that need mapped. If output have to use specific variable names and format, that's a different matter.

data/baseline/bmi_multi_realization_config_w_sloth_pet_cfe_nc.json Outdated
"registration_function": "register_bmi_cfe",
"variables_names_map": {
"atmosphere_water__liquid_equivalent_precipitation_rate": "precip_rate",
"atmosphere_air_water~vapor__relative_saturation": "SPFH_2maboveground",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove atmosphere_air_water~vapor__relative_saturation to land_surface_air__pressure as none of these are used by CFE. You may also need to map the PET output var here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been removed. PET output var is mapped. But may not have to, since it appears PET and CFE use the same variable name.

data/baseline/bmi_multi_realization_config_w_sloth_pet_cfe_nc.json Outdated
"main_output_variable": "Q_OUT",
"registration_function": "register_bmi_cfe",
"variables_names_map": {
"atmosphere_water__liquid_equivalent_precipitation_rate": "precip_rate",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is precip_rate the correct variable name from the forcing engine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

precip_rate used to be directly taken from forcing. This is because example forcing data provide the values for this variable. Not enough attention has been paid to this as focus has been on code development. Using output variable QINSUR fron NoahOWP is apparently more proper to couple NoahOWP to CFE. Otherwise, NoahOWP has no effect.

data/baseline/bmi_multi_realization_config_w_sloth_pet_noah_cfe_nc.json Outdated
"forcing_file": "",
"init_config": "",
"allow_exceed_end_time": true,
"main_output_variable": "QINSUR",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. QINSUR is the land surface water flux from Noah-OWP-Modular, which is not the final model in the stack. I think Q_OUT is what you want here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. this has been corrected.

data/baseline/bmi_multi_realization_config_w_sloth_pet_noah_cfe_nc.json Outdated
"variables_names_map": {
"PRCPNONC": "atmosphere_water__liquid_equivalent_precipitation_rate",
"Q2": "atmosphere_air_water~vapor__relative_saturation",
"SFCTMP": "land_surface_air__temperature",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice we have multiple conventions of mapping the forcing variables. Here, the mapping uses the BMI standard name convention (land_surface_air__temperature), while it previously used what I believe to be the AORC variable names (TMP_2maboveground). I assume this means that the forcing provider works with both conventions. However, I reckon providing both cases here without explanation will cause some confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the mappings has been removed.

data/baseline/bmi_multi_realization_config_w_sloth_pet_noah_cfe_nc.json Outdated
"main_output_variable": "Q_OUT",
"registration_function": "register_bmi_cfe",
"variables_names_map": {
"water_potential_evaporation_flux": "ETRAN",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ETRAN should be replaced by EVAPOTRANS. The former is transpiration and the latter is evapotranspiration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and I apologize for the confusing NOM names)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been changed.

data/baseline/bmi_multi_realization_config_w_sloth_pet_noah_cfe_nc.json Outdated
"allow_exceed_end_time": true,
"main_output_variable": "Q_OUT",
"registration_function": "register_bmi_cfe",
"variables_names_map": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add the QINSUR mapping to this list. For CFE it should be:

"atmosphere_water__liquid_equivalent_precipitation_rate": "QINSUR",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion.

@stcui007
Copy link
Contributor Author

stcui007 commented Apr 3, 2024 via email

@SnowHydrology
Copy link
Contributor

The realization configs were based on historically built previous files which may have been there a long time without updating.

That is correct. I've opened issue #787 and will work on updates.

@stcui007 stcui007 marked this pull request as ready for review April 9, 2024 13:18
@hellkite500 hellkite500 mentioned this pull request May 3, 2024
Open
6 tasks
@stcui007
Copy link
Contributor Author

stcui007 commented Jul 2, 2024

The realization configs have had much changes and several additions involving soil models. They have all been tested to run successfully with ngen framework. Much appreciated if someone interested in taking a look.

@PhilMiller PhilMiller mentioned this pull request Jul 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SnowHydrology SnowHydrology SnowHydrology requested changes

@ajkhattak ajkhattak Awaiting requested review from ajkhattak

@hellkite500 hellkite500 Awaiting requested review from hellkite500

@donaldwj donaldwj Awaiting requested review from donaldwj

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stcui007 @hellkite500 @SnowHydrology