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

pre-CFE2.0_PR #120

Merged
merged 27 commits into from
Jun 6, 2024
Merged

pre-CFE2.0_PR #120

merged 27 commits into from
Jun 6, 2024

Conversation

ajkhattak
Copy link
Contributor

@ajkhattak ajkhattak commented May 13, 2024

This PR is based on PR #116 and the current master. It wraps up the full capability of Nash Cascade-based surface runoff with backward compatibility. An example config and realization files are provided to turn ON Nash_Cascade surface runoff scheme. The old scheme GIUH-based surface runoff has been retained, and is the default option.

Based on the discussion with Fred, the commit 6d08db7bec2ec6cc2cc2446300c9e0d2076e0fc7 should be tagged as CFE1.0, and the new version will be CFE2.0 (previously we were mistakenly calling it CFE3.0)

NOTE: CFE2.0 defaults to CFE1.0.

Additions

Removals

  • None

Changes

Testing

  1. All existing tests have been tested, and old and new results are identical.
  2. All configuration files have been tested to work on both CFE1.0 and CFE2.0

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

…th_m to infiltration_excess_m in the Schaake and Xinanjiang schemes) and other minor changes.
…th_m to infiltration_excess_m in the Schaake and Xinanjiang schemes) and other minor changes.
…ficit, tested satdk as well (but not appropriate for channel infiltration due to small fraction of wetted area), removed linear K_infiltration due to non-linearity issues, and c1 being 2-3 order of magnitude smaller than c0.
@madMatchstick
Copy link
Contributor

@ajkhattak - I was able to run the new realization_cfe_pet_surfnash_calib.json after typeo fix - thanks! Although I did not look into results just yet.

Noting that the ngen integration tests use the existing giuh default option; realization_cfe_pet_surfgiuh.json - just a file name change for clarity.

@ajkhattak
Copy link
Contributor Author

@madMatchstick realization_cfe_pet_surfnash_calib.json will be added to CI, along with a few other minor changes, once this PR is done.
To compare the results of cfe1.0 vs this PR, I would use our four tests, if those results look okay, we are good.

@madMatchstick
Copy link
Contributor

madMatchstick commented May 22, 2024

@ajkhattak - Very minor recommended changes:

  1. Install.md - go ahead and include the correct cfe lib path based on realizations, CI, etc. So,
cmake -B extern/cfe/cfe/cmake_build -S extern/cfe/cfe/ -DNGEN=ON
make -C extern/cfe/cfe/cmake_build
  1. Question - why are we building fortran bmi for this example? We aren't coupling with NOM? Also not seeing instructions for PET lib build? Little confused here...
  2. Probably best to change "model_type_name": "bmi_multi_noahowp_cfe" to something like "bmi_multi_cfe_pet" for good measure. Here & here.
  3. Update bmi unit test to include 18th param K_infiltration newly added.

-Thanks!

Copy link
Contributor

@madMatchstick madMatchstick left a comment

Choose a reason for hiding this comment

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

@ajkhattak - Thanks for the recent commits. I do not see any major problems with this PR. We should probably start using releases for code versioning?

@ajkhattak ajkhattak changed the title CFE2.0_PR pre-CFE2.0_PR May 28, 2024
@ajkhattak ajkhattak merged commit 43f6b28 into master Jun 6, 2024
4 checks passed
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.

2 participants