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

ajk/cfe modifications #448

Merged
merged 4 commits into from
Oct 11, 2022
Merged

ajk/cfe modifications #448

merged 4 commits into from
Oct 11, 2022

Conversation

ajkhattak
Copy link
Contributor

The PR updates a few realization files to include SLoTH bmi that is now needed to run cfe in the ngen framework due to recent changes made to cfe. The PR does not change any existing functionality so No changes to the source code and results are consistent with that of the master branch.

Additions

  • Included SLoTH BMI to multi_bmi formulations in the realization files.

Removals

  • None

Changes

  • No changes to the source code

Testing

  1. All updated realization files are tested on MacOS and Linux (UCS2) machines

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 ➡️

@@ -46,27 +46,61 @@
"cat-27": {
"formulations": [
{
"name": "bmi_c",
"name": "bmi_multi",
Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, tricky... this file exists I think to give an example of a bmi_c formulation... but essentially CFE can't now run by itself with existing forcings.... so it has to exist in _multi ... can we make an example using exclusively libpetbmi instead? @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.

I see what you mean, yes we can't run cfe by itself (as a standalone bmi_c) within the framework anymore and we will need an example of bmi_c formulation, could be PET or noah-owp-mod

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked to the group this morning and we will probably just file a TODO to move these examples to the test modules, e.g. extern/test_bmi_c.

@@ -46,27 +46,61 @@
"cat-27": {
"formulations": [
{
"name": "bmi_c",
"name": "bmi_multi",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.... one way or another though, this file can actually be removed since #430.

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the above comment, we should have the same TODO clean up this file and make the other one OS-agnostic.

@mattw-nws
Copy link
Contributor

@ajkhattak: @donaldwj is going to have a quick look for any impacts to the CFE update that might not show up in the tests and after that we will merge.

@ajkhattak
Copy link
Contributor Author

sounds good to me. @rlmcdaniel and I have also done detailed testing to make sure cfe base functionality is not impacted.

@mattw-nws
Copy link
Contributor

#453 opened to address any further action here. Cursory review of the model changes does not raise any concerns. Merging.

@mattw-nws mattw-nws merged commit b7f5323 into master Oct 11, 2022
@mattw-nws mattw-nws deleted the ajk/cfe-modifications branch October 11, 2022 19:13
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