-
Notifications
You must be signed in to change notification settings - Fork 320
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
FATES parteh scale fixes api (ie nutrient + methane coupling) #1371
Conversation
…g maximum rooting depth bc into fates prior to use
ran aux_clm, still interpreting output, see here: /glade/scratch/rgknox/clmed-tests/parteh-scaling-fixes-api-Cbf358079-Fb77a0d5d-v3.aux_clm.cheyenne I did have to make some tweaks to the new fates-ch4 test, see the commit following this message. I re-ran that specific test and things seem to be working now: /glade/scratch/rgknox/ERS_Ld30.f45_f45_mg37.I2000Clm50FatesCru.cheyenne_intel.clm-FatesCH4.20210510_203248_jl6kjw/ |
Imported new parameter file:
|
… less resource intensive.
Addressed some issues brought up in the ctsm software meeting:
|
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.
I have a lot of nit picky things here. Some of it I just want to hear about your thinking. And as we have already discussed I'm fine with making an initial implementation that we don't change much, but start going toward a design we agree on.
An important issue that relates to this is #1051. We want to think of this as a step going towards some of that. |
After looking at the implementation. I do think that there should be some filters setup for running over FATES points versus vegetation (but NOT FATES) points. In most cases there should be two loops one loop for each. I only saw one instance where right now there might be issues with that since there's a part that they both do within the same loop. |
updated test results: |
Here are the izumi tests: /scratch/cluster/rgknox/tests_0522-093537iz From what I can tell, results between this branch (merged with dev039) and dev039 are identical (with exceptions to fates tests) and NLCOMPs. Although I'm not sure if the nlcomps are failing because of some write permission issues, or something else. |
Tests on izumi: /scratch/cluster/rgknox/tests_0524-115727iz From discussions with @glemieux , the nag fales with Clm50Fates D are known
|
The failed RUN tests on izumi were due to a "dangling pointer". The nuopc tests are expected fails. @rgknox thinks he knows the solution so he's trying it out. If it looks good we'll go ahead with the tag. Otherwise, it might take more time to get it out... |
Updated tests on izumi and cheyenne. (merged in dev040, and performed compare): Cheyenne: /glade/scratch/rgknox/tests_0525-131115ch I couldn't find any tests that had unexpected fails. Summary all pass with following expected exceptions: Expected BASELINE AND NLCOMP fails for all fates tests. Izumi (Vnuopc fails SHAREDLIB_BUILD): |
@ekluzek and I realized that the fates hash in the test, even though it was using the correct fates branch, had not merged in the most up-to-date version of fates. So I updated the external model pointer to use the new fates tag, and re-ran tests: cheyenne: /glade/scratch/rgknox/tests_0526-120450ch Update: ALL tests OK |
Description of changes
This set of changes is necessary to have API compatibility with FATES PR: NGEET/fates#716
In brief, these changes accomodate boundary conditions needed for nutrient coupling, although CLM-FATES nutrient coupling is forthcoming. These changes are also needed to enable methane coupling in with FATES-CLM which should be enabled now.
Specific notes
Tested this on cheyenne with modifications to check B4B, which generated expected results.
Here are the sub-branches to this PR that have the non-b4b changes suspended:
https://github.com/rgknox/fates/tree/parteh-scaling-fixes-may05-b4b
https://github.com/rgknox/ctsm/tree/fates_parteh_scale_fixes_api_b4b
Contributors other than yourself, if any: see NGEET/fates#716
CTSM Issues Fixed (include github issue #):
Are answers expected to change (and if so in what way)?
yes
Any User Interface Changes (namelist or namelist defaults changes)?
A new default parameter file has been loaded.
No new NL entries.
No changes in default NL entries.
use_fates and use_lch4 can now both be true.
Testing performed, if any:
The FATES test suite was run on the B4B sub-branches, results pass and are found here:
/glade/scratch/rgknox/clmed-tests/parteh-scaling-fixes-api-C7bd490c2-Fabb43f0b-test-b4b-v4.fates.cheyenne
(List what testing you did to show your changes worked as expected)
(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on cheyenne for intel/gnu and izumi for intel/gnu/nag/pgi is the standard for tags on master)
NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).