-
Notifications
You must be signed in to change notification settings - Fork 0
namespace changes (WIP) #3
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
base: fates-cn
Are you sure you want to change the base?
Conversation
slevis-lmwg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgknox this looks good to me. I can approve when we resolve my two questions.
| use PRTGenericMod, only : fates_cnp | ||
| use PRTGenericMod, only : carbon_only | ||
| use PRTGenericMod, only : carbon_nitrogen_phosphorus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you added a use statement for carbon_only that does not get used. If you did that intentionally, I'm fine with it. Otherwise, I would delete the unnecessary line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgknox while you resolve the other one, pls see what you think about this comment as well, though this one is just a case of an unnecessary line, so no big deal if it stays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and once these comments are resolved, this PR can be merged.
|
|
||
| ! initialize some values | ||
| hlm_parteh_mode = fates_c_only | ||
| hlm_parteh_mode = carbon_nitrogen_phosphorus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This replacement is incorrect, unless the fates_c_only here was incorrect to begin with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgknox pls correct me if I'm wrong: I think you said you would address this one, so I will wait.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
Though I also do not want to forget the two pending questions that I posted above: |
This fates side PR is paired with ctsm side PR slevis-lmwg/ctsm#19