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

bug_fix_aet rootzone #105

Merged
merged 2 commits into from
Feb 16, 2024
Merged

bug_fix_aet rootzone #105

merged 2 commits into from
Feb 16, 2024

Conversation

ajkhattak
Copy link
Contributor

The PR fixes bugs when using the AET Root Zone option. Currently, the rootzone block in the et_from_soil (in cfe.c) does not execute as aet_rootzone flag is always OFF. Other minor name changes..

Additions

  • None

Removals

  • None

Changes

  • name changes: sft_coupled to is_sft_coupled and aet_rootzone to is_aet_rootzone, also names with root_zone changed to rootzone (no underscore) for consistency

Testing

  1. tested the code to make sure the aet_rootzone option is used correctly

Notes

  • This bug was found while reviewing PR 104

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

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 - For good measure, I bounced these changes against sft & smp coupling as outlined in the sft install.md. ngen build/run was successful, although I didn't look into output/results much.

Note - there were some minor adjustments to the linked sft readme needed. Namely,

  1. adding the -DNGEN_IS_MAIN_PROJECT=ON flag to the noah-owp-modular lib build
  2. some pathing updates within realization_<*>.json & at runtime

I will make these changes directly in a sft pr but wanted to mention them here, as testing was apart of this review.

This cfe pr #105 is okay to merge. Thanks!

@ajkhattak
Copy link
Contributor Author

@madMatchstick thanks Jess for reviewing and testing with SFT and SMP, please submit a PR with your updates to SFT doc.

@ajkhattak ajkhattak merged commit 44bb30c into master Feb 16, 2024
4 checks passed
This was referenced Sep 6, 2024
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