Fixes to carbon accounting and lr2 machine files

[rgknox]

This pull request mostly deals with some bug fixes to carbon accounting. @jenniferholm noticed that the diagnostics for leaf_npp were periodically showing negative values. Through #164 we identified that this occurred because daily carbon balances during the allocation sequences were sometimes negative to to high respiration and low gpp. The model was correctly using storage carbon to "pay" the negative daily carbon balance and allow maintenance respiration to occur, it was not however correctly diagnosing this flow. The accounting of this process is a little complicated because storage can be used to pay for maintenance respiration as well as maintenance turnover demand.

During the process of verifying that carbon accounting errors were low, I triggered spurious values of output variables that triggered netcdf write errors, this lead to the identification that cohort%npp_accum was not being properly copied during copying of cohorts during patch fission.

To help in catching bad numerics from carbon balancing I added NaN and infinity traps to gpp, respiration and npp following photosynthesis, which should help rule out and quickly identify problems in the future.

A fix was needed to lawrencium machine files for its lr2 partition for serial runs. While these changes are unrelated to carbon accounting, they are trivial and simple, so I bundled them here.

Fixes: #164, #169, #168 and possibly #154

User interface changes?: no

Code review: requesting @jenniferholm and @serbinsh for evaluation in their science algorithms.

Test suite: lawrencium lr3 (baseline) and lawrencium-lr2 (non-baseline) edTest, Rapid Science Check tool (single site multi-decadal analysis)
Test baseline: 5c5928f
Test namelist changes: none
Test answer changes:
Test summary: all PASS

[rgknox]

@serbinsh, and @jenniferholm. As mentioned above, could you take a look at this PR and see if you can now meet the objectives that precipitated these fixes? Ie #164 and #154?

@bandre-ucar: I added some ieee_ checks. Could you take a look at those and give some feedback?

Of course, reviews from all on all aspects are welcome.

[jenniferholm]

@rgknox -- will do. I'll look at this PR and look at the storage carbon, leaf_npp, and carbon accounting. I might not get to this until Wednesday of this week, but will do. Thanks for looking into this!

[bandre-ucar]

@rgknox Are you referring to the huge and tiny calls in FatesConstants?

[rgknox]

@bandre-ucar I was thinking of the calls to check of Nans and Infs in EDAccumulateFluxesMod:

https://github.com/NGEET/ed-clm/pull/174/files#diff-d7a3799cd27da90352046ef429e56b5b

I originally was going to use manual NaN checks, ie seeing if a variable equaled itself. I was also going to test for infinity by seeing if the variable was > huge. I am no longer comparing to huge, since I am just calling the ieee intinsic functions, but I thought it couldn't hurt to leave those in there as parameters in-case we want to compare with them later on in different parts of the code.

[rgknox]

re-testing the merge with master on lawrencium

[bandre-ucar]

@rgknox Sorry I missed those. I would be extremely reluctant to use IEEE directly. The compilers have gotten much better about standards compliance and portability in the last few years, but... There's still a lot of old compilers in the wild. Take a look at the cime share library to see what is needed to make that truly portable:

https://github.com/NGEET/ed-clm/blob/master/cime/share/csm_share/shr/shr_infnan_mod.F90.in

My recommendation would be to use the share code. If fates doesn't want a dependency on cime, then copy the share code into fates and do a find replace 's/shr/fates/g'.

[rgknox]

Thanks for the recommendation @bandre-ucar. I'm going to just remove the addition of the checks for now. The checks are not central to this change, and deciding if we want compile dependencies on CIME is a worthy discussion topic.

[jenniferholm]

Hi Ryan,
I finished my initial review request using rgknox-leafnpp-diag-fix on lr3, looking into the carbon accounting. This is in a temperate forest, using two evergreen PFTs (comparable to my original test case where I noticed the incorrect diagnosing of carbon flow).

After these changes, I found in the diagnostics that negative NPP is now occurring the in the storage term and not leaf_npp. So that is corrected. I can provide comparison figures if that would help, just let me know. However the run crashes after about 40 years, and I cannot find a clear reason for the crash in the land or cesm log files. I also was testing a 4x5 resolution global run with this same branch for testing, and the case crashed in the first year due to NPP partitions not balancing ---

NPP Partitions are not balancing
Fractional Error: 0.256920533474132
Terms: 0.115500259887976 0.000000000000000E+000 7.554319516166393E-003
0.000000000000000E+000 2.474413364186671E-002 6.046502447631823E-003
4.748091589549142E-002
NPP components during FATES-HLM linking does not balance

On a side note, the total biomass, NPP etc. is lower now for this branch compared to my original case (which I believe did not have #138 yet), but I'm assuming that is because of other fixes to the maintenance respiration, correct (#138)?

@rgknox - how would you like me to proceed with testing, and trying to determine why my review request crashed?

[rgknox]

Thanks for the review @jenniferholm, this is very helpful.
Your results should indeed be significantly different if your previous version did not have the maintenance respiration changes.

As for the crash, this particular cohort has the following:
npp_total = 0.115500259887976
npp_leaf = 0.000000000000000E+000
npp_froot = 7.554319516166393E-003
npp_bsw = 0.000000000000000E+000
npp_bdead = 2.474413364186671E-002
npp_bseed = 6.046502447631823E-003
npp_store = 4.748091589549142E-002

The error claims that 2.9674388386819656E-002 of the carbon from NPP is not being accounted for in the partitions (this is just the error in the diagnostic, not the model integrations, btw)

This is a strange case. It is in positive carbon balance, so it must have leaves, yet... for some reason it does not send any carbon to leaves. Reasons to send NPP to leaves could be to replace losses due to turnover or phenology, or for growth. But the plant is incrementing dead biomass (growth increment) and seed, yet not incrementing sapwood. But.. npp is going to fine roots.

This seems like a deciduous tree that has leaves, but currently no leaf turnover, that was already "on allometry"? ie before this step its alive pools matched the maximums dictated by allometry...

But if I recall correctly, we don't have asynchronous ping-pong-like growth. I think if we have bdead increment, then we should have sapwood too..

I will look into this.

[rgknox]

Update: I think we have the problem solved. @jenniferholm made a test that included a deciduous PFT. This plant was encountering a portion of the code during a multi-year 4x5 spin-up test that we had not encountered. The portion of the code is the allocation of balive in the case where there is no leaf. The bug only had to do with diagnostics of npp_ flux partitions. I would like run a longer test, so I will check back in next week.
@bandre-ucar and @serbinsh, can we conclude your reviews, or are they on-going?
@serbinsh I was hoping that some of these changes might had fixed the crashes your were getting. Although I would like to push through and get these changes into master and keep your issue open if that is not the case.

[ckoven]

@serbinsh — have you had a chance to look at this PR yet to see if it fixes the problem you were having? thanks-

[jenniferholm]

@rgknox - Lawrencium was down yesterday, but I will begin testing the newest bug fixes in the next day or two .....(fyi).

[rgknox]

thanks @jenniferholm

[rgknox]

I believe this branch is ready for testing on yellowstone. I was able to reproduce @jenniferholm 's f45 test with 5 pfts, and finished a 20 year simulation without errors. We also verbally confirmed that the fixes to the diagnostics are resulting in leaf npp's that are positive only, and that negative npp's are being reflected in fluxes out of storage (which, by process, is covering maintenance demand).
I'm going to assign this to @bandre-ucar if I hear no qualms from @jenniferholm within the next day.

[bandre-ucar]

testing on yellowstone.