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

refactored cfe source file #76

Merged
merged 6 commits into from
Mar 22, 2023
Merged

refactored cfe source file #76

merged 6 commits into from
Mar 22, 2023

Conversation

ajkhattak
Copy link
Contributor

The PR separates the conceptual reservoir component from the CFE main source file. The refactored part is planned to be used in other models for ground water flows. The refactoring does not change the physics.

Additions

  • Added conceptual_reservoir.c and conceptual_reservoir.h files. No addition to the physics.

Changes

  • Changes to cmakelist file to add the source file of the conceptual reservoir. No changes to the physics.

Testing

  1. Tested all examples in the pseudo and nextgen frameworks. Results before and after refactoring are identical.

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)
  • New functions are documented (with a description, list of inputs, and expected output)
  • Reviewers requested with the Reviewers tool ➡️

@lcunha0118
Copy link
Contributor

@SnowHydrology I have reviewed the code and it looks good. I have not tested in NextGen since I am having some NextGen compiling issues. In my opinion is ready to merge.

include/cfe.h Outdated Show resolved Hide resolved
src/conceptual_reservoir.c Outdated Show resolved Hide resolved
@ajkhattak
Copy link
Contributor Author

@lcunha0118 once you fix your NextGen build issues and want to test it in the NextGen then make sure you checkout the associated branch ajk/cfe_gw_refactor. Once we merge this PR, I will submit a PR on ngen repo too.

@SnowHydrology
Copy link
Contributor

@ajkhattak, I'm finally reviewing this PR. Extracting the conceptual reservoir and coding it into its own file is good, but I'm concerned that we'll have the same problem as we had with the AORC forcing and PET: that we'll have identical code in multiple places that needs to be maintained (and this goes for the GIUH) as well. If we're going to use the GIUH and Nash-Cascade modules with other models we might want to maintain them in only one location. I'm curious what others think about making repos for the GIUH and Nash-Cascade so that we only have to maintain them in one location. @lcunha0118 @rlmcdaniel @mattw-nws

@ajkhattak
Copy link
Contributor Author

I think this is a valid point, we did talk about a placeholder for all these small modules but didn't reach to a conclusion. So I think this is probably the right time to discuss it. Since these are pretty small modules, so my only question would be should we keep them all in a generic repo or in separate repos.

@mattw-nws
Copy link

Generally I think the idea of a "x_utils" repo makes sense... possibly even a suite compilable to a .a lib or something (or just made a git extern and compiled in) ... naming things is hard ... what else would go in such a repo/library? This is probably all a little off topic, but, yes, I recommend getting these together into an easily reusable place.

@SnowHydrology
Copy link
Contributor

After the meeting on Friday, we are good to review and merge this PR. I've opened issue #80 for moving the GIUH and Nash-Cascade code to their own repo(s) at a later date.

@lcunha0118
Copy link
Contributor

lcunha0118 commented Mar 21, 2023

Ran in NextGen and results match with older version. The new version adds two new output variables: SOIL_STORAGE_CHANGE,SURF_RUNOFF_SCHEME

@lcunha0118 lcunha0118 closed this Mar 21, 2023
@lcunha0118 lcunha0118 reopened this Mar 21, 2023
src/conceptual_reservoir.c Outdated Show resolved Hide resolved
src/giuh.c Outdated Show resolved Hide resolved
src/cfe.c Outdated Show resolved Hide resolved
@madMatchstick madMatchstick merged commit bedc81b into master Mar 22, 2023
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.

6 participants