site-level mass flux accounting part 2

[rgknox]

Description:

This PR seeks to generate more consistency with how we track and restart some site-level mass fluxes. These mass fluxes are used for multiple purposes: run-time mass balance checks, history diagnostics and boundary conditions. So these fluxes needed a little attention in how they are used.

This PR is based on and should follow: #1448

Collaborators:

@glemieux @ckoven

Expectation of Answer Changes:

No answer changes, but it should fix restart errors with E3SM mass_balance_error diagnostics.

Checklist

If this is your first time contributing, please read the CONTRIBUTING document.

All checklist items must be checked to enable merging this pull request:

Contributor

• The in-code documentation has been updated with descriptive comments
• The documentation has been assessed to determine if updates are necessary

Integrator

• FATES PASS/FAIL regression tests were run
• Evaluation of test results for answer changes was performed and results provided
• FATES-CLM6 Code Freeze: satellite phenology regression tests are b4b

If satellite phenology regressions are not b4b, please hold merge and notify the FATES development team.

Documentation

• Technical Note update:
• User's Guide update:

Test Results:

CTSM (or) E3SM (specify which) test hash-tag:

CTSM (or) E3SM (specify which) baseline hash-tag:

FATES baseline hash-tag:

Test Output:

[rgknox]

Making a note that we should add a comment units fix originally posted #1474

[maritsandstad]

Sorry, I made #1474 by accident, but I guess FYI the actual PR I meant to make for our NorESM side stuff is here NorESMhub#31 with a bit of discussion for background, that include fixing the comment also for grazing which is wrong in the same way presumably for the same reason

[glemieux]

Looks good. I only have one question that might be more for my own education. Otherwise this is good to go from my perspective.

[glemieux]

Regression testing on derecho is underway.

[glemieux]

Regression testing against fates-sci.1.88.2_api.42.0.0-ctsm5.3.085 is mostly B4B with the following exceptions:

FAIL ERP_Ld9.f45_f45_mg37.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesColdAllVars BASELINE fates-sci.1.88.2_api.42.0.0-ctsm5.3.085: DIFF
FAIL ERS_Ld366.1x1_brazil.I2000Clm50FatesCruRsGs.derecho_intel.clm-FatesFireLightningPopDens BASELINE fates-sci.1.88.2_api.42.0.0-ctsm5.3.085: DIFF

Both are showing diffs only in one fates output:

3795  FATES_INTERR_LIVEVEG_EL   (lon,lat,fates_levelem,time)  t_index =      1     1
3796          26     3312  (    24,    33,     1,     1) (    10,    19,     1,     1) (     6,    15,     1,     1) (     4,    19,     1,     1)
3797                 1083   1.922704773171802E-10         -2.220446049250313E-16 5.6E-17  5.551115123125783E-17 1.6E-02  0.000000000000000E+00
3798                 1083   1.922704773171802E-10         -2.220446049250313E-16          1.110223024625157E-16          5.551115123125783E-17
3799                 3312  (    24,    33,     1,     1) (    10,    19,     1,     1)
3800           avg abs field values:    4.566315264762014E-13    rms diff: 8.2E-18   avg rel diff(npos):  1.6E-02
3801                                    4.566313983341164E-13                        avg decimal digits(ndif):  0.2 worst:  0.0
3802  RMS FATES_INTERR_LIVEVEG_EL          8.2205E-18            NORMALIZED  1.8002E-05

I think this may be an acceptible diff given that the changes to the calculation of net_uptake affect the calculation of iflux_liveveg here:

fates/main/ChecksBalancesMod.F90

Lines 318 to 319 in 546326c

	ibal%iflux_liveveg = ibal%iflux_liveveg + &
	( net_uptake &

Thoughts @rgknox ?

[glemieux]

I'm running a modified version of the sp mode compset adding FATES_INTERR_LIVEVEG_EL to see if this is still b4b since it's not nominally included in the history output for sp tests.

[glemieux]

I'm running a modified version of the sp mode compset adding FATES_INTERR_LIVEVEG_EL to see if this is still b4b since it's not nominally included in the history output for sp tests.

I realized this isn't necessary per the history output logic.

[glemieux]

I'm seeing DIFFS on the exact restart with the e3sm allvars testmod. Looking into this.

[mvdebolskiy]

Can this be an optional argument? With a local variable that is .false. by default and takes the argument value when the argument is present? Then you don't have to alter every call of this routine

[rgknox]

Sounds fine to me, I'll submit the change

[glemieux]

I'm seeing DIFFS on the exact restart with the e3sm allvars testmod. Looking into this.

@rgknox I figure this out. We need to move the bc_out%ar_site and bc_out%gpp_site prior to this line where the gpp_acc and aresp_acc are zero'd during restart:

fates/main/EDMainMod.F90

Lines 856 to 861 in 56add1b

	if(hlm_use_sp.eq.ifalse .and. (.not.is_restarting))then
	call canopy_spread(currentSite)
	else
	site_cmass%gpp_acc = 0._r8
	site_cmass%aresp_acc = 0._r8
	end if

which would result in the bc_out varaibles being zero'd out during restart. I'll submit a change shortly.

[mvdebolskiy]

@rgknox the last commit won't build, because of the argument name and local name conflicts.
present().
I'd sugget:
mvdebolskiy@5d3546d

[rgknox]

merged, thanks @mvdebolskiy

[rgknox]

Tests look good, merging

/glade/derecho/scratch/<>/tests_1129-120611de

Double checked diffs and they were only relegated to the error tracking diagnostic, which was the targetted change.