ajk/cfe icefraction #49

ajkhattak · 2022-02-22T20:01:14Z

Couples CFE with soil freeze-thaw (SFT) model
Changes to runoff schemes (Schaake and Xinanjiang)
Changes to BMI CFE input variables (new input variables are ice_fraction_schaake and ice_fraction_xinan) they both have different units
Changes to BMI CFE output variables (BMI outputs storage, storage change at the timestep)
Coupling requires CFE to provide/pass runoff scheme to CFE through BMI
Basic functionality of the CFE, when run without the SFT model, is unaffected by the changes

Additions

cat_*_bmi_cfe.txt files need to provide urban_decimal_fraction if runoff scheme is Xinanjiang

Changes

Submodules for runoff schemes take an additional argument (ice_fraction) now.
Number of variables output to screen/console now includes ice_fraction and total storage

Testing

Simulations using the pseudo framework make_and_run_bmi_pass_forcings_pet.sh give same results before and after the changes

Screenshots

Checklist

src/bmi_cfe.c

src/cfe.c

configs/cat_87_bmi_config_cfe_pass.txt

madMatchstick · 2022-03-01T20:35:37Z

Note PR48 - Configuration file and parameter read-in changes was just merged. All config files now include [units]. e.g. see cat_87_bmi_config_cfe.txt. @ajkhattak When fetching upstream, can you please account for this change as well as adding any appropriate [units] to new params in both cat_87_bmi_config_cfe.txt & cat_87_bmi_config_cfe_pass.txt? -thanks

… bm_cfe print header line.

…scheme when ice fraction is used are moved to config file.

src/cfe.c

…tent_threshold parameter unit.

src/cfe.c

madMatchstick · 2022-03-02T18:53:40Z

Suggest removing last # three lines from cat_87_*.txt (2) and including appropriate schaake only params in cat_58_*.txt (2) and/or cat_89_*.txt (3). Perhaps former where ice_fraction=1 & latter ice_fraction=0 (or vise versa)?

Best to maintain all files in /config with latest working version of bmi-cfe.

rlmcdaniel · 2022-03-02T19:50:20Z

I suggest updating the documentation. It may be as simple as noting the option for including SFT (e.g. how to include or disable SFT). But other parameters within CFE, functionality changes, etc. should also be included somewhere in the documentation.

ajkhattak · 2022-03-02T20:20:56Z

@madMatchstick I removed the three lines from cat_87 files. All other config files should work without any modifications. If CFE is not coupled to SFT, no additional parameters in the config files are needed.

madMatchstick · 2022-03-02T20:27:05Z

I suggest updating the documentation. It may be as simple as noting the option for including SFT (e.g. how to include or disable SFT). But other parameters within CFE, functionality changes, etc. should also be included somewhere in the documentation.

Noted, adding these changes to the cfe doc list! Thank you.

documenting changes to cfe config files when coupled SFT model.

…k within the ngen framework, also removed variables from the output list.

SnowHydrology · 2022-03-04T13:12:30Z

@ajkhattak, I propose we change the merge branch for this PR from master to a new branch called cfe_soilfreezethaw. Given the number of changes proposed and some of the issues uncovered (when running in Nextgen, for example), I think it would be best to keep development in a non-master branch for now.

Additionally, the master branch of CFE is essentially a stable version we can use for testing formulations. When building ngen, users can fetch this stable version through a simple one-liner (git submodule update --remote extern/cfe/cfe). Considering we do not yet support C++ or have two-way coupling in ngen, I feel that having the stable version include SFT modifications may cause some confusion.

If you're okay with this, I will modify the PR accordingly. (Suggestions welcome from others!)

SnowHydrology · 2022-03-04T20:36:42Z

I've changed the proposed merge branch to cfe_soilfreezethaw. We can work on cleaning up and merging this PR and then continue development (e.g., BMI modifications) in the new branch.

…scheme when ice fraction is used are moved to config file.

…k within the ngen framework, also removed variables from the output list.

madMatchstick

@SnowHydrology - just confirming that we are NOT requiring this version to run in ngen, correct? (Hence the move to non-master, dev, branch.)

README.md

SnowHydrology · 2022-03-09T20:56:18Z

@SnowHydrology - just confirming that we are NOT requiring this version to run in ngen, correct? (Hence the move to non-master, dev, branch.)

@madMatchstick, that is correct. Please merge if it passes your review.

ajkhattak · 2022-03-14T18:00:26Z

@SnowHydrology yes, Jess is correct. this branch won't run within the ngen framework due to coupling-related changes to bmi.

ajkhattak · 2022-03-15T02:51:19Z

Sorry for the last confusing comment from me. I misread it. Anyway, I am going to merge this PR into cfe_soilfreezethaw

* changes to include ice fraction to the cfe runoff schemes. * modified config file to add xinan urban fraction and minor changes to bm_cfe print header line. * changes suggested in the PR#49, hard-coded parameters in the schaake scheme when ice fraction is used are moved to config file. * added runoff ice_fraction parameters to cfe state variables. * added units to the config files, minor changes bmi related to ice_content_threshold parameter unit. * a minor change, moved cv_frz inside if condition. * cleanup, config files and cfe.c * Update README.md documenting changes to cfe config files when coupled SFT model. * Update README.md * Update README.md * typo in SFT section * Update README.md * removed ice_fraction variables from the bmi input list to make it work within the ngen framework, also removed variables from the output list. * Reverting to 180c56f to use it in the pseudo framework. * Adding ET to output * adding gw and soil storage to output * changes to include ice fraction to the cfe runoff schemes. * changes suggested in the PR#49, hard-coded parameters in the schaake scheme when ice fraction is used are moved to config file. * removed ice_fraction variables from the bmi input list to make it work within the ngen framework, also removed variables from the output list. * Reverting to 180c56f to use it in the pseudo framework. * name change: ice_fraction to sft_coupled. * cleanup/renaming output variables. * remove refs to `ngen` in README Co-authored-by: JessicaGarrett-NOAA <30940444+madMatchstick@users.noreply.github.com> Co-authored-by: lcunha0118 <luciana.k.cunha@gmail.com> Co-authored-by: Keith Jennings <keithjennings@keiths-mbp.lan>

* ajk/cfe icefraction (#49) * changes to include ice fraction to the cfe runoff schemes. * modified config file to add xinan urban fraction and minor changes to bm_cfe print header line. * changes suggested in the PR#49, hard-coded parameters in the schaake scheme when ice fraction is used are moved to config file. * added runoff ice_fraction parameters to cfe state variables. * added units to the config files, minor changes bmi related to ice_content_threshold parameter unit. * a minor change, moved cv_frz inside if condition. * cleanup, config files and cfe.c * Update README.md documenting changes to cfe config files when coupled SFT model. * Update README.md * Update README.md * typo in SFT section * Update README.md * removed ice_fraction variables from the bmi input list to make it work within the ngen framework, also removed variables from the output list. * Reverting to 180c56f to use it in the pseudo framework. * Adding ET to output * adding gw and soil storage to output * changes to include ice fraction to the cfe runoff schemes. * changes suggested in the PR#49, hard-coded parameters in the schaake scheme when ice fraction is used are moved to config file. * removed ice_fraction variables from the bmi input list to make it work within the ngen framework, also removed variables from the output list. * Reverting to 180c56f to use it in the pseudo framework. * name change: ice_fraction to sft_coupled. * cleanup/renaming output variables. * remove refs to `ngen` in README Co-authored-by: JessicaGarrett-NOAA <30940444+madMatchstick@users.noreply.github.com> Co-authored-by: lcunha0118 <luciana.k.cunha@gmail.com> Co-authored-by: Keith Jennings <keithjennings@keiths-mbp.lan> * changed bmi input variable ice_fraction name. * bug fix, smc_ref is dimensionless so dividing by the soil reservoir depth. * separated giuh module from cfe to have its own source/header files, so it can be reused and extented, if needed, independently. * root zone adjusted AET * updates to match smc code changes * Update laramie_bmi_config_smc_coupler.txt * Rename src/main.c to src/old_driver_scripts/main.c * Rename src/main_cfe_aorc_pet.c to src/old_driver_scripts/main_cfe_aorc_pet.c * Rename src/main_pass_forcings.c to src/old_driver_scripts/main_pass_forcings.c * Delete env_cheyenne.sh * Delete make_and_run_bmi.sh * Delete make_and_run_bmi_pass_forcings.sh * Delete make_and_run_bmi_pass_forcings_pet.sh * Update CMakeLists.txt * Update README.md * Update README.md * minor changes, updated files due to changes in the SMP. * removed sft_coupled=on, not needed * updated CMakeLists and config files, the merge/changes reproduce cfe standalone results with basecase, aorc forcings only, aorc forcings + pet, and aorc + forcing + pet + aet root zone. * added bash script to run cfe with different options. documenation needs to be updated. * set default smc profile to 1x1 array, needed for cfe to run in the ngen framework as soil_moisture_profile is a bmi input. * bug fix, assigns 0 to n_soil_layers when rootzone AET is turned off. * added the right file for AET rootzone example. * bug fix, set default aet_root_zone to FALSE. * Update README.md individual "make and run" files have been replace with a single file. For all examples, cmake is needed to build the code. * Update README.md * Update README.md * Update README.md * minor text edit. * Added option to turn on/off AET rootzone and other related changes. * few fixes that were introduced during rebasing. * Update README.md * Update README.md * modified run_cfe to catch/output invalid option. * Update README.md * Update README.md * added an explicit flag to check if SFT is ON or OFF inside the runoff schemes, if OFF ice_fraction parameters is set to 0.0. Updated readme file. * Update README.md * Update README.md * minor changes in response to PR comments. * Update README.md Co-authored-by: JessicaGarrett-NOAA <30940444+madMatchstick@users.noreply.github.com> Co-authored-by: lcunha0118 <luciana.k.cunha@gmail.com> Co-authored-by: Keith Jennings <keithjennings@keiths-mbp.lan> Co-authored-by: Rachel McDaniel <rachel.mcdaniel@outlook.com> Co-authored-by: Rachel McDaniel <61248457+rlmcdaniel@users.noreply.github.com>

ajkhattak requested review from hellkite500, lcunha0118 and rlmcdaniel February 22, 2022 20:01

madMatchstick reviewed Feb 23, 2022

View reviewed changes