Complete the FATES-CLM nitrogen coupling#3409
Complete the FATES-CLM nitrogen coupling#3409slevis-lmwg wants to merge 46 commits intoESCOMP:masterfrom
Conversation
PASS ERS_D_Ld30.1x1_brazil.I2000Clm60FatesCrujraRs.derecho_intel.clm-FatesColdPRT2 FAIL ERS_D_Ld30.1x1_brazil.I2000Clm60FatesCrujraRs.derecho_intel.clm-FatesColdPRT2--clm-mimicsFatesCold--clm-nofireemis The latter needs "nofireemis" to work with Fates, but it then dumps core in line 1180 SoilBiogeochemDecompCascadeMIMICSMod.F90
...calculating these variables: nf_soil%decomp_npools_sourcesink_col nf_soil%fates_litter_flux
|
With the latest commit, I repeated the two earlier tests and added another to check whether answers have changed from the baseline: The latter fails in case2, after reading the restart file, with a N balance error. UPDATE
|
|
Enabled fixation and ran the same tests: PRT2 still b4b with the baseline. PASS The same tests with the code change for harvest (same results relative to the baseline). |
|
Worked on the next checkbox in the issue (#3378), submitted the same tests, and after some troubleshooting: |
These variables originate in fates, so this renaming requires the same renaming in fates; I will open the corresponding PR very soon
Notes:
|
|
Updated the fates paramfile (see next commit) and submitted these two again The first (LUH2) same as before (DIFF from baseline) since nothing changed for it.
|
Simplify messaging during docs build slevis resolved conflicts: bld/namelist_files/namelist_defaults_ctsm.xml
|
Testing on derecho after the latest updates:
Before I try
|
|
Now repeat
Known issue #3840 |
|
@rgknox testing seems satisfactory to me, so this PR and its fates counterpart are ready for your final review, unless you think of anything else that we need to work on here. |
glemieux
left a comment
There was a problem hiding this comment.
This looks good. Thanks again to @slevis-lmwg for the walkthrough. I just have two questions/comments that should be dealt with via other PRs (if at all).
| 'FATES_DEMOTION_CARBONFLUX', 'FATES_PROMOTION_CARBONFLUX', | ||
| 'FATES_MORTALITY_CFLUX_CANOPY', 'FATES_MORTALITY_CFLUX_USTORY', | ||
| 'FATES_NEP', 'FATES_HET_RESP', 'FATES_FIRE_CLOSS', 'FATES_FIRE_FLUX_EL', | ||
| 'FATES_CBALANCE_ERROR', 'FATES_ERROR_EL', 'FATES_LEAF_ALLOC', |
There was a problem hiding this comment.
Do we have an issue to note why this shouldn't be included here? I could find anything specific outside of a conflict with using it during ST3 or SP mode tests.
| <test name="ERS_D_Ld30" grid="f45_f45_mg37" compset="I2000Clm60FatesRs" testmods="clm/FatesColdPRT2_suplnAll"> | ||
| <machines> | ||
| <machine name="derecho" compiler="intel" category="fates"/> | ||
| <machine name="lawrencium-lr3" compiler="intel" category="fates"/> |
There was a problem hiding this comment.
@rgknox do you use lawrencium? We should instances of lawrencium if we don't think we're going to be using it anytime soon, but that's out of scope for this PR so I'd just make an issue if you agree.
Description of changes
For now see the issue #3378
Corresponding mods on the FATES side:
NGEET/fates#1472
Nutrient enabled FATES handbook
FATES CLM N coupling
Specific notes
Contributors other than yourself, if any:
@rgknox @adrifoster @wwieder
CTSM Issues Fixed (include github issue #):
#3378
Are answers expected to change (and if so in what way)?
For non-fates tests expect roundoff diffs due to a change in the order of operations in CNNDynamicsMod.F90 as documented below.
Any User Interface Changes (namelist or namelist defaults changes)?
Yes, see changes to CLMBuildNamelist and namelistdefaults.
Does this create a need to change or add documentation? Did you do so?
Yes, no.
Initial testing performed
...with the first two commits in this PR:
Later comments point out that these two tests were inadequate at catching problems, and that I switched to two other tests.