Remove excess initialization from BGC code #6058

peterdschwartz · 2023-11-06T23:18:38Z

Every model timestep, the vegetation and column fluxes are set to zero for the CNP variables.
This took up roughly 1/3rd of the time for the Ecosystem model time, and this PR reduces that by over half.

[BFB]

Added initializing into compute loops when needed, and switched some variables to be only be initialized on the first time step. the harvest variables shouldn't require initializing every timestep but extra effort is needed to untangle the dependencies and order of operations.

…lity. Cleaned up the comments in VegetationDataType

…land test to fail.

rgknox · 2023-11-17T16:05:10Z

components/elm/src/biogeochem/SoilLittDecompMod.F90

@@ -308,7 +308,9 @@ subroutine SoilLittDecompAlloc (bounds, num_soilc, filter_soilc,    &
                             / cp_decomp_pools_new(c,j,cascade_receiver_pool(k)) )

                     else   ! 100% respiration
-                        pmpf_decomp_cascade(c,j,k) = - p_decomp_cpool_loss(c,j,k) / cp_decomp_pools(c,j,cascade_donor_pool(k))
+                        if(decomp_ppools_vr(c,j,cascade_donor_pool(k)) > 0._r8) then


@peterdschwartz, is this future proofing or was this already causing div0s? If zero's were being passed in, then I would recommend flagging this as an issue so you can have someone put time into figuring out why.

No, this only became an issue when I removed certain initializations from the SetValues routine. Likewise with other changes: some initializations were moved to loops that first use that variable. This is for variables that only have meaningful values for certain types of pfts or cols for example.

@rgknox Did I answer your question?

yes, sorry for the late reply! thanks for the bump

rgknox

These look good to me @peterdschwartz

components/elm/src/data_types/ColumnDataType.F90

peterdschwartz · 2024-01-05T16:16:50Z

addressed comments

…#6058) Every model timestep, the vegetation and column fluxes are set to zero for the CNP variables. This took up roughly 1/3rd of the time for the Ecosystem model time, and this PR reduces that by over half. [BFB]

peterdschwartz · 2024-01-08T18:15:31Z

I see some of the e3sm_integration BGC tests have diffs with intel compiler only. I'll try to have it sorted today

rljacob · 2024-01-08T20:16:35Z

Looks like it was a small number of fields.

peterdschwartz · 2024-01-08T22:50:40Z

@bishtgautam Queue times have been slow, but I've isolated the issue to a specific subroutine. I'll be able to push a fix sometime tonight (depending on job turnover), but I wouldn't worry about being able to re-merge it tonight.

… next (PR #6058)" This reverts commit 575f773, reversing changes made to ab7c7e0.

rljacob · 2024-01-10T04:18:13Z

Testing found some diffs with baselines. Seen on both chrysalis and pm-cpu (The only ones running integration with baselines)

The cases with 5 variables diffing.

SMS_Ld2.ne30pg2_r05_EC30to60E2r2.BGCEXP_CNTL_CNPECACNT_1850.*_*.elm-bgcexp
SMS_Ld2.ne30pg2_r05_EC30to60E2r2.BGCEXP_CNTL_CNPRDCTC_1850.*_*.elm-bgcexp

The variables (from one of the cases, grepping for "RMS" in cprnc.out)

 RMS x2a_Fall_fco2_lnd                5.6027E+32            NORMALIZED  1.1852E+01
 RMS a2x_Sa_co2prog                   4.2255E+41            NORMALIZED  1.3157E+01
 RMS l2x_Fall_fco2_lnd                1.9829E+33            NORMALIZED  3.6984E+00
 RMS x2l_Sa_co2prog                   1.5536E+42            NORMALIZED  4.0495E+00
 RMS x2oacc_Sa_co2prog                6.1847E+40            NORMALIZED  1.9795E+01

This seems like its a simple fix.

But 2 other cases had 255 variables diffing (which implies a real diff introduced in to the state early since its only 5 steps).
The cases:

SMS_Ln5.ne30pg2_r05_EC30to60E2r2.BGCEXP_LNDATM_CNPRDCTC_1850.*_*
SMS_Ln5.ne30pg2_r05_EC30to60E2r2.BGCEXP_LNDATM_CNPRDCTC_20TR.*_*

peterdschwartz · 2024-01-10T14:20:31Z

@rljacob Yes I'm aware of the differences. I tried to focus on

SMS_Ln5.ne30pg2_r05_EC30to60E2r2.BGCEXP_LNDATM_CNPRDCTC_1850.*_*

as it is shorter and requires less resources, but I ran into the issue on chrysalis where this test DIFFs (in 255 variables) with master. I asked Gautam to do the same, and his case also gave DIFFs (same 255 variables) with master, so I think something is wrong with the default chrysalis environment (this isn't the only issue I've encountered when running test suites on chrysalis)

I don't have this issue with pm-cpu but the queue times are pretty long and that's the only thing holding this debugging effort back.

rljacob · 2024-01-10T17:14:14Z

I was just noting for the record.

The jenkins testing on Chrysalis uses different node counts then regular runs for a given resolution. If you are comparing a run you do with the Jenkins-generated baseline then you might be picking up a pelayout diff that has crept in. Try making your own baseline with the same pelayout as your test case.

peterdschwartz · 2024-01-10T17:30:25Z

@rljacob Right! the cases do also report NLDIFF for the pelayout so that could be it, thanks.

rljacob · 2024-01-10T19:13:48Z

Good that should help find the issue with this PR. Unfortunately, there's now another issue because that case can't pass a PEM test. That may have existed for a while and can be looked at later.

peterdschwartz · 2024-01-16T18:12:02Z

@bishtgautam Newest commit has fixed the 4 test diffs

…ns' into next (PR #6058)"" This reverts commit dccaffe.

…#6058) Re-merging the PR #6058 Every model timestep, the vegetation and column fluxes are set to zero for the CNP variables. This took up roughly 1/3rd of the time for the Ecosystem model time, and this PR reduces that by over half. [BFB]

rljacob · 2024-01-17T20:32:20Z

@bishtgautam you can merge this to master.

peterdschwartz added 5 commits October 31, 2023 15:28

Made sure certain mortality variables are set to zero in the GapMorta…

de984a1

…lity. Cleaned up the comments in VegetationDataType

Reverted my change assigning nstep to a known value as that causes a …

61edeb5

…land test to fail.

Cleaned up commented out lines.

61fede1

cleaned up debugging code segments

7886cd2

rljacob added Land BGC labels Nov 7, 2023

rljacob assigned bishtgautam Nov 7, 2023

rljacob requested a review from rgknox November 7, 2023 18:59

rgknox reviewed Nov 17, 2023

View reviewed changes

rgknox approved these changes Nov 28, 2023

View reviewed changes

bishtgautam reviewed Jan 4, 2024

View reviewed changes

components/elm/src/data_types/ColumnDataType.F90 Outdated Show resolved Hide resolved

bishtgautam reviewed Jan 4, 2024

View reviewed changes

components/elm/src/data_types/ColumnDataType.F90 Outdated Show resolved Hide resolved

Removed code that had been commented out.

772a5de

bishtgautam self-requested a review January 5, 2024 22:45

bishtgautam approved these changes Jan 5, 2024

View reviewed changes

rljacob added this to the v3.0.0-rc milestone Jan 8, 2024

bishtgautam added a commit that referenced this pull request Jan 9, 2024

Revert "Merge branch 'peterdschwartz/lnd/remove-initializations' into…

dccaffe

… next (PR #6058)" This reverts commit 575f773, reversing changes made to ab7c7e0.

col_cf%nee needs to be zero'd every time step for BGC coupled runs.

90a2790

bishtgautam added a commit that referenced this pull request Jan 16, 2024

Revert "Revert "Merge branch 'peterdschwartz/lnd/remove-initializatio…

e2b7700

…ns' into next (PR #6058)"" This reverts commit dccaffe.

bishtgautam merged commit 057f1f8 into E3SM-Project:master Jan 17, 2024
3 checks passed

peterdschwartz mentioned this pull request Jan 31, 2024

Add testmods to fates regression test to increase coverage and include long-term exact restart fix #6018

Merged

rljacob modified the milestones: v3.0.0-rc1, v3.0.0-rc2, v3.0.0, v3.0.0-rc Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove excess initialization from BGC code #6058

Remove excess initialization from BGC code #6058

peterdschwartz commented Nov 6, 2023 •

edited by bishtgautam

Loading

rgknox Nov 17, 2023

peterdschwartz Nov 17, 2023

peterdschwartz Nov 28, 2023

rgknox Nov 28, 2023 •

edited

Loading

rgknox left a comment

peterdschwartz commented Jan 5, 2024

peterdschwartz commented Jan 8, 2024

rljacob commented Jan 8, 2024

peterdschwartz commented Jan 8, 2024

rljacob commented Jan 10, 2024 •

edited

Loading

peterdschwartz commented Jan 10, 2024 •

edited

Loading

rljacob commented Jan 10, 2024 •

edited

Loading

peterdschwartz commented Jan 10, 2024

rljacob commented Jan 10, 2024

peterdschwartz commented Jan 16, 2024

rljacob commented Jan 17, 2024

Remove excess initialization from BGC code #6058

Remove excess initialization from BGC code #6058

Conversation

peterdschwartz commented Nov 6, 2023 • edited by bishtgautam Loading

rgknox Nov 17, 2023

Choose a reason for hiding this comment

peterdschwartz Nov 17, 2023

Choose a reason for hiding this comment

peterdschwartz Nov 28, 2023

Choose a reason for hiding this comment

rgknox Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

rgknox left a comment

Choose a reason for hiding this comment

peterdschwartz commented Jan 5, 2024

peterdschwartz commented Jan 8, 2024

rljacob commented Jan 8, 2024

peterdschwartz commented Jan 8, 2024

rljacob commented Jan 10, 2024 • edited Loading

peterdschwartz commented Jan 10, 2024 • edited Loading

rljacob commented Jan 10, 2024 • edited Loading

peterdschwartz commented Jan 10, 2024

rljacob commented Jan 10, 2024

peterdschwartz commented Jan 16, 2024

rljacob commented Jan 17, 2024

peterdschwartz commented Nov 6, 2023 •

edited by bishtgautam

Loading

rgknox Nov 28, 2023 •

edited

Loading

rljacob commented Jan 10, 2024 •

edited

Loading

peterdschwartz commented Jan 10, 2024 •

edited

Loading

rljacob commented Jan 10, 2024 •

edited

Loading