Fix C balance and FPE errors #6235

peterdschwartz · 2024-02-13T18:33:45Z

Carbon balance was never checked correctly and let through negative values.
Initialization of Ecosystem variables needed to be re-adjusted, and balance checking for FATES needed to be changed, which causes round-off differences in CMASS_BALANCE_ERROR for fate_cold_allvars test.

Also included divide-by-zero check in PhenologyMod.F90 to fix fpe in debug mode.

[non-BFB] for one FATES test.
Fixes #6120
Fixes #6203
Fixes #6177

peterdschwartz · 2024-02-13T18:42:21Z

@glemieux Here is a PR to fix the balance checking issues. I did get DIFF for fates_cold_allvars but only for CMASS_BALANCE_CHECK but the values were still ~1E-16.

Let me know if how I implemented the new balance checking makes sense to you.

glemieux

The implementation of the fates gridcell balance checks looks good to me. Thanks!

peterdschwartz · 2024-02-21T15:02:01Z

After doing a test merge I obtained FILLDIFF for the fates_cold_allvars. That I need to look into today

peterdschwartz · 2024-02-21T23:38:06Z

Quick update: fates_cold_allvars FAILs at the COMP_base_rest stage. The cprnc file just shows CMASS_BALANCE_ERROR with very small differences. Since there are no other diffs (i.e., in variables used to calculate carbon balance), I wonder if it's just due to floating point rounding issues. But, if that is the case, I'm not sure what the solution should be.

 CMASS_BALANCE_ERROR   (lndgrid,time)  t_index =     10    10
        127      384  (    25,     1) (    93,     1) (   148,     1) (   148,     1)
                 211   0.000000000000000E+00  -2.810582373931857E-16 2.3E-21 -1.292301226966941E-16 2.0E-06 -1.292301226966941E-16
                 211   0.000000000000000E+00  -2.810570991926628E-16         -1.292278595305381E-16         -1.292278595305381E-16
                 384  (    25,     1) (    93,     1)
          avg abs field values:    1.589865621873051E-16    rms diff: 0.0E+00   avg rel diff(npos):  2.0E-06
                                   1.589865886570847E-16                        avg decimal digits(ndif):  5.7 worst:  4.8
 RMS CMASS_BALANCE_ERROR              0.0000E+00            NORMALIZED  0.0000E+00

rljacob · 2024-02-21T23:44:05Z

What is the diff with? A baseline from the previous code or is it a diff within a test like an ERS test?

peterdschwartz · 2024-02-21T23:53:17Z

Not baseline diff (though that is also there but makes sense to me). It fails the COMP_base_rest portion of ERP test (same thing with the ERS version I made for debugging).

rljacob · 2024-02-22T00:23:06Z

Try a REP test.

peterdschwartz · 2024-02-22T17:20:35Z

REP passes just fine. I'm going to double check that each variable for carbon balance is read/written to the restart file, and maybe a longer simulation will reveal diff's accruing in other fields.

bishtgautam

I have minor stylistic code change requests for this PR.

bishtgautam · 2024-03-20T01:42:12Z

components/elm/src/biogeochem/EcosystemBalanceCheckMod.F90

@@ -624,7 +621,7 @@ subroutine ColPBalanceCheck(bounds, &
      ! set time steps
      dt = dtime_mod
      kyr = year_curr; kmo = mon_curr; kda = day_curr; mcsec = secs_curr;
-
+      !nstep = nstep_mod 


Please delete this line.

bishtgautam · 2024-03-20T01:43:08Z

components/elm/src/biogeochem/EcosystemBalanceCheckMod.F90

+         grc_coutputs              => grc_cf%coutputs              , & ! Output: [real(r8) (:) ] (gC/m2/s) column-level C outputs
+         beg_totpftc               =>  grc_cs%beg_totpftc          , & ! Input: [real(r8) (:)] (gC/m2) patch-level carbon aggregated to column level, incl veg and cpool
+         beg_cwdc                  =>  grc_cs%beg_cwdc             , & ! Input: [real(r8) (:)] (gC/m2) total column coarse woody debris carbon
+         beg_totsomc               =>  grc_cs%beg_totsomc          , & ! Input: [real(r8) (:)] (gC/m2) total column soil organic matter carbon
+         beg_totlitc               =>  grc_cs%beg_totlitc          , & ! Input: [real(r8) (:)] (gC/m2) total column litter carbon
+         beg_totprodc              =>  grc_cs%beg_totprodc         , & ! Input: [real(r8) (:)] (gC/m2) total column wood product carbon
+         beg_ctrunc                =>  grc_cs%beg_ctrunc           , & ! Input: [real(r8) (:)] (gC/m2) total column truncation carbon sink
+         beg_cropseedc_deficit     =>  grc_cs%beg_cropseedc_deficit  & ! Input: [real(r8) (:)] (gC/m2) column carbon pool for seeding new growth


Please remove an extra whitespace after => grc_cs% to be => grc_cs%

bishtgautam · 2024-03-20T01:43:42Z

components/elm/src/biogeochem/EcosystemBalanceCheckMod.F90


+      if(use_fates) then 


please add a whitespace if (use_fates)

bishtgautam · 2024-03-20T01:43:56Z

components/elm/src/biogeochem/EcosystemBalanceCheckMod.F90

      dt = real( get_step_size(), r8 )
      nstep = get_nstep()

      do g = bounds%begg, bounds%endg
-         grc_cinputs(g) = grc_gpp(g) + grc_dwt_seedc_to_leaf(g) + grc_dwt_seedc_to_deadstem(g)
+
+         if(.not. use_fates) then 


similarly add a whitespace.

bishtgautam · 2024-03-20T01:44:13Z

components/elm/src/biogeochem/PhenologyMod.F90

         ! calculate the P:PET for each month
-         p2ETo(p,kmo) = xp(p,kmo)/ETo(p,kmo)
+         if( abs(ETo(p,kmo)) > 0._r8) then 


please add whitespace

Shouldn't there also be code corresponding to an else condition?

bishtgautam · 2024-03-20T01:46:23Z

components/elm/src/data_types/ColumnDataType.F90

+       this%er(i)                        = value_column !! Grid Balance Error 
+       this%som_c_leached(i)             = value_column !! Grid Balance Error 


Can you please remove the !! Grid Balance Error

peterdschwartz · 2024-03-21T16:37:10Z

status:
fates_cold_allvars still fails restart test. Tried adding every gridcell and column variable related to CMASS_BALANCE to history and restart files but no other differences. Tried running test for 2 years to accumulate more carbon, but there was no other diffs detected.

Adjusting the history output interval did allow the test to pass the restart comparison but that's not a real solution.

Scheduled meeting with FATES devs to go over the logic for balance checking with FATES enabled.

rljacob · 2024-04-12T04:04:13Z

@peterdschwartz what is the status of this PR?

peterdschwartz · 2024-04-12T16:18:17Z

@rljacob I met with @glemieux a couple of weeks ago. He mentioned a possible time-step dependent part of FATES that could be the cause, but I think he's been busy with the FATES api update and our follow up meeting with @thorntonpe was cancelled due to lab holiday.

Today, I'll try increasing the history output to be every timestep and see if that provides any useful information.

peterdschwartz · 2024-04-12T18:32:26Z

Update:

Increasing output freq finally showed a new diff in a field. Looking into it further, it appears that the vertical transport tendency terms for carbon are not added to the corresponding leaching term due to some issue 5 years ago (according to git blame atleast), but it is for nitrogen and phosphorus.

Hopefully the straightforward solution works here.

 LITR2C_TNDNCY_VERT_TRANS   (lndgrid,levdcmp,time)  t_index =    194   194
          3     5760  (   153,     2,     1) (   153,     1,     1) (   237,     3,     1) (   247,    10,     1)
                3165   1.043116371057806E-09  -1.643725044964128E-09 2.2E-19  2.944499461962624E-12 8.1E-11  1.293930177203629E-16
                3165   1.043116371057806E-09  -1.643725044964128E-09          2.944499678803059E-12          1.293930309552527E-16
                5760  (   153,     2,     1) (   153,     1,     1)
          avg abs field values:    4.034842251976478E-11    rms diff: 0.0E+00   avg rel diff(npos):  8.1E-11
                                   4.034842251976478E-11                        avg decimal digits(ndif):  7.1 worst:  7.0
 RMS LITR2C_TNDNCY_VERT_TRANS         0.0000E+00            NORMALIZED  0.0000E+00

glemieux · 2024-04-17T05:21:35Z

@peterdschwartz can you point me to the line of code with the git blame that you noted earlier, please?

peterdschwartz · 2024-04-17T15:58:56Z

@glemieux This Code block is what I was referring to. The transport tendency is not added to carbon leached pool even though the corresponding terms are added for Nitrogen and Phosphorus.

More experimenting showed that bypassing the SoilLittVertTransp calculation, the restart failure goes away. For FATES, the UpdateLitterFluxes is done just prior that adjusts the inputs to the SoilLittVertTransp that could be the cause. Line

rljacob · 2024-05-02T17:34:57Z

Waiting to see about FATES side of this PR.

peterdschwartz · 2024-05-21T19:26:44Z

Since there's a previous fates related issue linked above with the same errors, I'm going to just clean the branch up for the other issues and follow the review comments. The other issue can be used to the issue.

glemieux · 2024-05-22T19:23:51Z

Since there's a previous fates related issue linked above with the same errors, I'm going to just clean the branch up for the other issues and follow the review comments. The other issue can be used to the issue.

Sorry for the delay in trying to address the fates-side of this. Just to clarify, you're saying your going to move forwards towards integration @peterdschwartz? If so, that sounds good to me.

Corrected GridCBalanceCheck to use absolute value for error. Adjusted col_cf_setvalues to fix undetected C imbalances -- Still error with FATES

…n input to col_cf%litfall Added more output when an carbon imbalance is detected. Column Carbon balance is now checked with FATES active.

Adjusted whitespaces and comments.

peterdschwartz · 2024-05-23T15:47:08Z

Since there's a previous fates related issue linked above with the same errors, I'm going to just clean the branch up for the other issues and follow the review comments. The other issue can be used to the issue.

Sorry for the delay in trying to address the fates-side of this. Just to clarify, you're saying your going to move forwards towards integration @peterdschwartz? If so, that sounds good to me.

@glemieux Yes, and now I know how to how to reliably reproduce the issue, we can use #6125 to track progress. I will share a branch with the changes to do it.

@bishtgautam I have rebased the branch and addressed your comments. There's potentially a couple of different ways to handle the else condition in the PhenologyMod, so I went with what I believe to be the simplest and added a comment explaining why. Let me know what you think

e3sm_developer came back [BfB] with the exception of one FATES test that will FAIL restart comparison for the time being.

bishtgautam · 2024-05-23T18:44:20Z

components/elm/src/biogeochem/PhenologyMod.F90

+         if ( abs(ETo(p,kmo)) > 0._r8) then 
+            p2ETo(p,kmo) = xp(p,kmo)/ETo(p,kmo)
+         else ! P:PET is undefined.
+            ! Setting to a fill value ( 'spval' ) would
+            ! require nested if statements due to 
+            ! the weighting of previous years (i.e., p2ETo and prev_p2ETo_bar )
+            ! So, set to zero for simplicity.
+            p2ETo(p,kmo) = 0._r8
+         end if 


@peterdschwartz This looks good to me. Thanks for the comment explaining the logic.

rljacob · 2024-05-30T17:37:07Z

This is ready to merge.

) Carbon balance was never checked correctly and let through negative values. Initialization of Ecosystem variables needed to be re-adjusted, and balance checking for FATES needed to be changed, which causes round-off differences in CMASS_BALANCE_ERROR for fate_cold_allvars test. Also included divide-by-zero check in PhenologyMod.F90 to fix fpe in debug mode. [non-BFB] for one FATES test. Fixes #6120 Fixes #6203 Fixes #6177

…ixed

) Re-merging the PR as a FATES test has been moved to a different test suite.

peterdschwartz requested a review from glemieux February 13, 2024 18:35

peterdschwartz added Land bug fix PR labels Feb 13, 2024

rljacob assigned bishtgautam Feb 15, 2024

rljacob added the FATES label Feb 15, 2024

rljacob assigned peterdschwartz and unassigned bishtgautam Feb 15, 2024

glemieux approved these changes Feb 15, 2024

View reviewed changes

glemieux approved these changes Feb 16, 2024

View reviewed changes

rljacob assigned bishtgautam and unassigned peterdschwartz Feb 19, 2024

peterdschwartz mentioned this pull request Feb 21, 2024

Fixes CNP Budget #6233

Merged

bishtgautam requested changes Mar 20, 2024

View reviewed changes

glemieux mentioned this pull request Apr 17, 2024

Exact restart failure for f45_g37 grid for SOIL2N_TNDNCY_VERT_TRANS for comprehensive FATES testmod #6125

Open

peterdschwartz added 3 commits May 22, 2024 20:42

Fixed divide-by-zero in crop model.

4850808

Corrected GridCBalanceCheck to use absolute value for error. Adjusted col_cf_setvalues to fix undetected C imbalances -- Still error with FATES

Adjusted GridCBalanceCheck when FATES is active to set gridcell carbo…

a111285

…n input to col_cf%litfall Added more output when an carbon imbalance is detected. Column Carbon balance is now checked with FATES active.

Adjusted setting of p2ETo in PhenologyMod.F90

e6097d2

Adjusted whitespaces and comments.

peterdschwartz force-pushed the peterdschwartz/lnd/fix-balance-errors branch from cac574e to e6097d2 Compare May 23, 2024 15:41

bishtgautam reviewed May 23, 2024

View reviewed changes

bishtgautam self-requested a review May 30, 2024 17:37

bishtgautam approved these changes May 30, 2024

View reviewed changes

Moved fates_land_developer test to fates test suite until it can be f…

7b9ce63

…ixed

bishtgautam added a commit that referenced this pull request Jun 4, 2024

Merge branch 'peterdschwartz/lnd/fix-balance-errors' into next (PR #6235

3797ed8

) Re-merging the PR as a FATES test has been moved to a different test suite.

bishtgautam merged commit db11f6c into E3SM-Project:master Jun 5, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix C balance and FPE errors #6235

Fix C balance and FPE errors #6235

peterdschwartz commented Feb 13, 2024 •

edited

Loading

peterdschwartz commented Feb 13, 2024

glemieux left a comment •

edited

Loading

peterdschwartz commented Feb 21, 2024

peterdschwartz commented Feb 21, 2024

rljacob commented Feb 21, 2024

peterdschwartz commented Feb 21, 2024

rljacob commented Feb 22, 2024

peterdschwartz commented Feb 22, 2024

bishtgautam left a comment

bishtgautam Mar 20, 2024

bishtgautam Mar 20, 2024

bishtgautam Mar 20, 2024

bishtgautam Mar 20, 2024

bishtgautam Mar 20, 2024

bishtgautam Mar 20, 2024

peterdschwartz commented Mar 21, 2024 •

edited

Loading

rljacob commented Apr 12, 2024

peterdschwartz commented Apr 12, 2024

peterdschwartz commented Apr 12, 2024 •

edited

Loading

glemieux commented Apr 17, 2024

peterdschwartz commented Apr 17, 2024

rljacob commented May 2, 2024

peterdschwartz commented May 21, 2024

glemieux commented May 22, 2024

peterdschwartz commented May 23, 2024

bishtgautam May 23, 2024

rljacob commented May 30, 2024

		this%er(i) = value_column !! Grid Balance Error
		this%som_c_leached(i) = value_column !! Grid Balance Error

Fix C balance and FPE errors #6235

Fix C balance and FPE errors #6235

Conversation

peterdschwartz commented Feb 13, 2024 • edited Loading

peterdschwartz commented Feb 13, 2024

glemieux left a comment • edited Loading

Choose a reason for hiding this comment

peterdschwartz commented Feb 21, 2024

peterdschwartz commented Feb 21, 2024

rljacob commented Feb 21, 2024

peterdschwartz commented Feb 21, 2024

rljacob commented Feb 22, 2024

peterdschwartz commented Feb 22, 2024

bishtgautam left a comment

Choose a reason for hiding this comment

bishtgautam Mar 20, 2024

Choose a reason for hiding this comment

bishtgautam Mar 20, 2024

Choose a reason for hiding this comment

bishtgautam Mar 20, 2024

Choose a reason for hiding this comment

bishtgautam Mar 20, 2024

Choose a reason for hiding this comment

bishtgautam Mar 20, 2024

Choose a reason for hiding this comment

bishtgautam Mar 20, 2024

Choose a reason for hiding this comment

peterdschwartz commented Mar 21, 2024 • edited Loading

rljacob commented Apr 12, 2024

peterdschwartz commented Apr 12, 2024

peterdschwartz commented Apr 12, 2024 • edited Loading

glemieux commented Apr 17, 2024

peterdschwartz commented Apr 17, 2024

rljacob commented May 2, 2024

peterdschwartz commented May 21, 2024

glemieux commented May 22, 2024

peterdschwartz commented May 23, 2024

bishtgautam May 23, 2024

Choose a reason for hiding this comment

rljacob commented May 30, 2024

peterdschwartz commented Feb 13, 2024 •

edited

Loading

glemieux left a comment •

edited

Loading

peterdschwartz commented Mar 21, 2024 •

edited

Loading

peterdschwartz commented Apr 12, 2024 •

edited

Loading