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.

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SnowHydrology
Copy link
Contributor

#787 indicates that the example realization configs in the data directory are out of date. Notably, none of the realizations mapped Noah-OWP-Modular QINSUR 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

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • 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

  • Linux

@SnowHydrology
Copy link
Contributor Author

I set this as draft because I have a couple questions:

  • @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 but not in example_bmi_multi_realization_config__macos.json here.
  • @hellkite500 Is the test_realization_config.json config used anymore? I noticed it has t-shirt parameters, which seems out of date.

@stcui007
Copy link
Contributor

stcui007 commented Apr 4, 2024 via email

@SnowHydrology
Copy link
Contributor Author

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.

@stcui007
Copy link
Contributor

stcui007 commented Apr 4, 2024 via email

@ajkhattak
Copy link
Contributor

@SnowHydrology

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 looks to me like the updated version that has the correct SLoTH+CFE mapping, so we should be using this one.

this example is outdated.

Just a note: NONE of the soil models (other than the topmodel) will run in the framework without SLoTH outputs.

@ajkhattak
Copy link
Contributor

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.

@SnowHydrology One clarification here, CFE takes ice_fraction_xinanjiang as an input from SFT, however, if SFT is not coupled than it comes from SLoTH. So a user through SLoTH might, by mistake, set it to a non-zero number so these lines

if(!soil_reservoir_struct->is_sft_coupled)
	  soil_reservoir_struct->ice_fraction_xinanjiang = 0.0;

ensure that ice_fraction_xinanjiang is zero if SFT is not coupled otherwise it will lead to incorrect runoff calculations.

Copy link
Contributor

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?)

Copy link
Contributor Author

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",
Copy link
Contributor

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.

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 catch. Fixed.

Copy link
Contributor

@ajkhattak ajkhattak left a 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",
Copy link
Contributor

Choose a reason for hiding this comment

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

ETRAN -> EVAPOTRANS??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hellkite500, please confirm

@stcui007
Copy link
Contributor

I have done tests running ngen with all realizations except example_bmi_multi_realization_config__macos.json and example_bmi_multi_realization_config_w_routing.json in both serial and parallel mode, as the automatic test has only limited coverage. They all run successfully on local Linux servers. Maybe Keith alreay done this. Then this just confirm it.

@SnowHydrology
Copy link
Contributor Author

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.

@stcui007
Copy link
Contributor

stcui007 commented Jul 2, 2024

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 test_... realization configs, I believe they are not used currently. They were probably meant to test the configurations with some explicit specification for some cat-id.

The example_bmi_multi_realization_config_w_routing.json might possibly be out of date and be removed as well.

In realizations involve both PET and CFE, depending on preference, the following line of mapping is not absolutely necessary, I think.

"water_potential_evaporation_flux": "potential_evapotranspiration"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants