Use reproducible global sums when BFBFLAG is set by billsacks · Pull Request #704 · ESCOMP/CMEPS

billsacks · 2026-09-17T16:02:29Z

Description of changes

There are a few places in CMEPS where global sums are performed. These global sums can be sensitive to processor count. This causes failures in tests - like the ERP test - that check for bit-for-bit results with changing processor count.

This PR resurrects the BFBFLAG xml variable and uses it to choose whether to perform global sums in the original way or using shr_reprosum_mod to get bit-for-bit results with changing processor count. (This is done via a bfbflag attribute; this attribute is in Allcomp so CDEPS can also leverage it.) By default we still use the original way, both for better performance and because UFS doesn't have shr_reprosum_mod. (Note that an error will be raised if trying to set bfbflag with UFS.) Some CIME tests, such as the ERP test, turn on BFBFLAG so will leverage this new behavior.

This is implemented in three places: prep_glc (for SMB renormalization), post_rof (for removal of negative runoff) and prep_atm (for an enthalpy correction). There is one other possibly relevant global sum in CMEPS - for med_diag_mod - which I am not handling here; I think this is just for budget diagnostics, which I feel are okay to differ by roundoff with processor changes, and from a quick look, I think that fixing that would require more extensive changes.

Note that, in order to leverage the new routine that computes global sums in either the old or new way (depending on bfbflag), I needed to change the code to put the summands in arrays rather than computing local sums, because the reproducible sum routine requires us to not do any pre-summation on local processors. (An exception is summation across elevation classes on a single grid cell: that summation is safe to do ahead of time since it doesn't depend on processor count.)

Most of the changes were written by Claude, but with detailed guidance and careful reviews by myself.

Specific notes

Contributors other than yourself, if any:

CMEPS Issues Fixed (include github issue #):

Resolves Leverage BFBFLAG to use reproducible sums in place of ESMF_VMAllreduce #650

Are changes expected to change answers? (specify if bfb, different at roundoff, more substantial) - roundoff-level differences for some configurations:

Potential roundoff-level changes from baseline for all configurations with DGLC or CISM (due to splitting the sum over elevation classes and the sum over grid cells into two different steps)
Potential roundoff-level changes from baseline for tests that turn on BFBFLAG (ERP, PEM, PET, SEQ), since this flag is now enabled whereas previously it did nothing

Any User Interface Changes (namelist or namelist defaults changes)? Adds bfbflag attribute

Testing performed

aux_cime_baselines and aux_cdeps in the context of cesm3_0_beta09, with this branch and the associated CDEPS branch.

All tests pass and are bit-for-bit except expected failures and this test that has roundoff-level changes: SMS_Ly2.f09_g17_gris20.T1850Gg.derecho_intel. By putting in place some temporary changes, I verified that these roundoff-level changes are due to the splitting of sums over elevation classes and sums over grid cells in the new code, changing the order of operations from before.

Also some additional tests to give better test coverage; these included the following (along with some others that were less good tests, so I'm not documenting them here)

SMS_Ld3.ne30pg3_t232.B1850C_LTso.derecho_intel.drv-glc_avg_frequently - bit-for-bit with baseline
ERP_P64x2_Ld366.f10_f10_mg37.I2000Clm50BgcCrop.derecho_intel.clm-irrig_alternate_monthly - roundoff-level diffs from baseline; bit-for-bit with baseline if I set BFBFLAG FALSE.

I also ran some tests with BFBFLAG TRUE vs. FALSE to confirm that we get only roundoff-level diffs from setting BFBFLAG:

SMS_D_Ld3.f19_g17.X.green_gnu.drv-glc_avg_frequently
SMS_Ld3.f19_g17.X.green_gnu.drv-glc_avg_frequently
SMS_D_Ld3.f19_g17.X.green_gnu.drv-water_tracers--drv-glc_avg_frequently
SMS_Ld3.f19_g17.X.green_gnu.drv-water_tracers--drv-glc_avg_frequently

For all of these, I made changes (from a branch that will be coming in soon) that change xrof to send negative as well as positive runoff (to better exercise the post_rof code). For the water_tracers tests, I also had to put this in user_nl_cpl:

water_tracers_variables_not_checked = "FBExprof%Flrl_irrig_wtracers:FBExpocn%Foxx_rofi_wtracers:FBExpocn%Foxx_rofl_wtracers"

These tests showed just roundoff-level diffs between BFBFLAG true and false, as expected.

If BFBFLAG is set (e.g., by the ERP, PEM, PET and SEQ tests, or by the user), then compute these global sums in a way that is bit-for-bit independent of processor count.

We'll need this in CDEPS (DGLC) as well

The motivation for this is so that we have nearly-identical modules between CMEPS and CDEPS for ease of diffability.

This allows these global sums to be reproducible with different processor counts if bfbflag is set. Code changes made by Cladue, reviewed carefully by myself.

cmeps1.1.68

cmeps1.1.69

billsacks · 2026-09-17T16:03:17Z

I want to do a little more testing to verify that this is all working as expected... I'll post here when that's complete.

Katetc · 2026-09-17T21:06:08Z

I just wanted to start my review by saying this line in the PR Summary:

Potential roundoff-level changes for tests that turn on BFBFLAG (ERP, PEM, PET, SEQ)

Feels kind of ironic. I understand that you mean the initial answers, not the restarted (repro_sum'd) answers, but maybe it's worth qualifying which answers are BFB and which are roundoff-level changed.

Katetc · 2026-09-17T21:12:47Z

+    <file>env_run.xml</file>
+    <desc>
+      If TRUE, perform some calculations (in CMEPS and elsewhere) such as global sums in a
+      manner that is bit-for-bit independent of processor count.


"If TRUE, perform some calculations (in CMEPS and elsewhere) such as global sums using a less efficient algorithm that is independent of processor count. This allows for Bit-For-Bit (BFB) restarts after changing PE layouts, for many compsets. However, setting this flag does NOT guarantee that all calculations throughout all model configurations are BFB independent of processor count."

I have partially reworded this based on your suggestion (e151743). I left out the mention of "restarts" because I felt that was a bit confusing: Even though this shows up in restart tests (ERP) it is not directly related to restarts. See if you're happy with the new wording in that commit.

billsacks · 2026-09-17T21:27:37Z

I just wanted to start my review by saying this line in the PR Summary:

Potential roundoff-level changes for tests that turn on BFBFLAG (ERP, PEM, PET, SEQ)

Feels kind of ironic. I understand that you mean the initial answers, not the restarted (repro_sum'd) answers, but maybe it's worth qualifying which answers are BFB and which are roundoff-level changed.

Point taken. I tried adjusting this in the top-level PR description. See if this reads better now.

Katetc

There's nothing here that is a show stopper. I've added some wording suggestions in a few places, and just thought that adding some checks around allocate and deallocate calls are usually a good idea. But, we are in a time crunch, so feel free to just mark those resolved if it's not worth making changes.

Katetc · 2026-09-17T21:14:38Z

+      If true, perform some calculations (in CMEPS and elsewhere) such as global sums in a
+      manner that is bit-for-bit independent of processor count.
+      (This does NOT guarantee that all calculations throughout the model are bit-for-bit
+      independent of processor count.)


I find it a little weird that this text is in two places, but whatever. If you like my suggestion above, you could update this one as well.

I find it a little weird that this text is in two places, but whatever. If you like my suggestion above, you could update this one as well.

Yeah, I hear you. Since both of them will appear in the documentation in separate places - depending on whether you are looking at the documentation of xml variables or namelist variables - I thought it was worth documenting it in both places. I suppose I could just say, "see documentation of BFBFLAG xml variable". Would you prefer that?

I think it's fine to have the same text in both places. Redundancy should generally be avoided, but it's not the end of the world. :D

Katetc · 2026-09-17T21:31:23Z

+
+    ! local variables
+    ! Note that summands_2d is deliberately allocatable, rather than being an automatic array or
+    ! an inline reshape of local_summands: those would give an array temporary, which some


give an array temporary... memory?

give an array temporary... memory?

You're right - that wording is bad. I changed it to "create an array temporary", which is what it means.

I was on the fence about whether to keep this comment at all. Claude added it when I asked Claude to change the implementation to avoid creating an array temporary. I almost deleted it, feeling it was unnecessary, but then decided that it provides a little value. Looking back on it, I am tempted to delete it. Let me know if you have preferences.

I support deleting this comment. I'm glad you are thinking about these things, but you don't have to defend every piece of code from potential criticism.

Katetc · 2026-09-17T21:34:44Z

+
+    rc = ESMF_SUCCESS
+
+    allocate(summands_2d(size(local_summands), 1))


Ok, I was feeling like a check was needed here, and I figured out what I was thinking is adding a return stat in the allocate call and checking to make sure it was successful. Like:
`allocate(summands_2d(size(local_summands), 1), stat=ierr)

if (ierr /= 0) then
errstring='Error allocating summands_2d'`

I realize that feelings differ across CSEG on this point (with CAM apparently preferring a stat check). I personally prefer to not have an explicit check of the stat of an allocate: if you don't query the stat, then the code will abort if there's an error in allocation; if you do query the stat, then it's up to you to handle the error. This leaves open the possibility that you'll make an error in the error handling itself, accidentally letting an allocation error slip through without aborting. I think I've seen this kind of bug in the error handling of an allocate in a recent code review. I realize that catching the error can allow for a more informative error message, but my feeling is that it's not worth the extra code and the possibility of inserting a bug in the error-handling code.

I'm open to having my mind changed on this point, but I think this is a bigger discussion that should be taken outside of this PR review: the vast majority of allocates in CMEPS do not check the stat, so I prefer to stay consistent with the pre-existing pattern until we have a discussion where the consensus is that we want to add this pattern.

Hi Bill, no worries. I think the fact that this is in-line with CMEPS standards is good enough. I've had too many reviews where CAM SEs (no need to name names!) would flag all of mine, so I just want to make sure we at least consider the choice.

Katetc · 2026-09-17T21:35:31Z

-    local_positive(1) = 0.0_r8
-    local_negative(1) = 0.0_r8
+    allocate(local_positive(size(runoff_flux)))
+    allocate(local_negative(size(runoff_flux)))


Maybe a check for error status on the allocate?

Katetc · 2026-09-17T21:36:21Z

-    if (ChkErr(rc,__LINE__,u_FILE_u)) return
-    global_sum = global_positive(1) + global_negative(1)
+    deallocate(local_positive)
+    deallocate(local_negative)


Maybe a check to make sure it's allocated before deallocating?

Maybe a check to make sure it's allocated before deallocating?

This is another conversation that I think should be taken outside this PR review if you think it's important: From my quick look, it looks like we rarely check the allocated status of arrays before deallocating in CMEPS, and it seems unnecessary here because they are allocated unconditionally. (I have a feeling that the places that do check the allocation status are handling situations where the array may truly be unallocated at that point - for example, I see this in a "clean" routine that I think I may have written, which is handling the possibility that "clean" is called - e.g., from a unit test - despite the array not having been allocated.) So this would be a broader discussion on the preferred pattern in general.

Actually, I think it would be safe to completely remove these deallocate statements, because since Fortran 2003 allocatables are automatically deallocated when they go out of scope. I thought about doing that, and would personally prefer removing them completely over adding a conditional check around the deallocate.

Hmm, TIL. Well, most of my Fortran has this check in it, both CISM and CAM. Mostly in case the code changes in a way that could result in the array being unallocated when the deallocate is called. I agree it's not super necessary here.

Hmm, TIL. Well, most of my Fortran has this check in it, both CISM and CAM. Mostly in case the code changes in a way that could result in the array being unallocated when the deallocate is called. I agree it's not super necessary here.

Fair enough. I see that rationale. Personally, I think I'd look at that and say, "why is this only conditionally deallocated? what is the situation where it wouldn't have been allocated?" But I certainly see your perspective on this, too, and it's a good one.

Katetc · 2026-09-17T21:38:03Z

-      local_positive_final(1) = 0.0_r8
-      local_negative_final(1) = 0.0_r8
+      allocate(local_positive_final(size(runoff_flux)))
+      allocate(local_negative_final(size(runoff_flux)))


Safety checks on these allocates?

Katetc · 2026-09-17T21:38:32Z

      if (ChkErr(rc,__LINE__,u_FILE_u)) return
-      global_sum_final = global_positive_final(1) + global_negative_final(1)
+      deallocate(local_positive_final)
+      deallocate(local_negative_final)


And checks here as well

Katetc · 2026-09-17T21:39:45Z

+      deallocate(local_positive_final)
+      deallocate(local_negative_final)
+      global_sum_final = global_positive_final + global_negative_final
      if (maintask) then


So, outside the scope of these changes, but the writes above included "dbug_flag > dbug_threshold" in this check and I think that might be good here to reduce log output.

This whole block of code is already inside a conditional on dbug_flag > dbug_threshold.

Katetc · 2026-09-17T21:42:50Z

-    ! determine accumulation and ablation sums for qice on the land grid
+    ! determine accumulation and ablation summands for qice on the land grid
+    allocate(local_accum_lnd(size(lndfrac), 1+num_wtracers))
+    allocate(local_ablat_lnd(size(lndfrac), 1+num_wtracers))


I'm going to say "Safety checks?" here as well, but I'm starting to doubt myself.

Katetc · 2026-09-17T21:43:35Z

+    call med_global_sums(gcomp, local_ablat_lnd, global_ablat_lnd, rc)
    if (ChkErr(rc,__LINE__,u_FILE_u)) return
+    deallocate(local_accum_lnd)
+    deallocate(local_ablat_lnd)


Check to see if allocated first

billsacks · 2026-09-17T23:33:41Z

@Katetc - thank you very, very much for your quick turnaround of this! I have addressed some of your comments with improved wording in comments / descriptions (thanks for those suggestions!). I replied to your comments about adding checks around allocates / deallocates. I think I have addressed everything at this point, at least if you're happy with how I addressed them.

I'm still running some final testing... derecho's slow queues have been slowing me down today.

billsacks · 2026-09-18T18:25:44Z

I have done some additional testing, which I have documented in the top-level comment of this PR.

billsacks · 2026-09-18T19:50:19Z

@Katetc gave me the go-ahead to merge....

Use reproducible global sums in DGLC when bfbflag is set ### Description of changes The global sums in DGLC lead to roundoff-level differences with different processor counts. This causes failures in tests - like the ERP test - that check for bit-for-bit results with changing process count. This PR uses the bfbflag attribute to choose whether to perform global sums in the original way or using shr_reprosum_mod to get bit-for-bit results when changing processor count. This bfbflag attribute is added in ESCOMP/CMEPS#704, and the changes in this PR are connected to that CMEPS PR. By default we still use the original way, both for better performance and because UFS doesn't have shr_reprosum_mod. (Note that an error will be raised if trying to set bfbflag with UFS.) Some CIME tests, such as the ERP test, turn on BFBFLAG so will leverage this new behavior. The global sums routine is duplicated between CMEPS and CDEPS. I don't like that. But I couldn't see a good alternative. Alternatives I considered were: - Have cmeps use the version from cdeps. However, this both (a) seems like it might not work in all build environments, and (b) feels like a weird dependency - Putting this in CESM_share. But then, since UFS doesn't use CESM_share, we would need a wrapper on the UFS side that would involve about the same amount of duplication that we currently have. Most of the changes were written by Claude, but with detailed guidance and careful reviews by myself. ### Specific notes Contributors other than yourself, if any: CDEPS Issues Fixed (include github issue #): Are there dependencies on other component PRs (if so list): - Depends on ESCOMP/CMEPS#704 Are changes expected to change answers (bfb, different to roundoff, more substantial): roundoff-level changes for tests that set BFBFLAG and use DGLC (ERP, PEM, PET, SEQ) Any User Interface Changes (namelist or namelist defaults changes): Testing performed (e.g. aux_cdeps, CESM prealpha, etc): aux_cime_baselines and aux_cdeps in the context of cesm3_0_beta09, with this branch and the CMEPS branch in ESCOMP/CMEPS#704. All tests pass and are bit-for-bit except expected failures and this test that has roundoff-level changes due to a change in CMEPS: `SMS_Ly2.f09_g17_gris20.T1850Gg.derecho_intel`. Also some additional tests to give better test coverage; these included the following (along with some others that were less good tests, so I'm not documenting them here) - SMS_Ld3.ne30pg3_t232.B1850C_LTso.derecho_intel.drv-glc_avg_frequently - bit-for-bit with baseline - ERP_P64x2_Ld366.f10_f10_mg37.I2000Clm50BgcCrop.derecho_intel.clm-irrig_alternate_monthly - roundoff-level diffs from baseline; bit-for-bit with baseline if I set BFBFLAG FALSE.

billsacks added 9 commits August 11, 2026 10:03

If BFBFLAG is set, compute glc renormalization sums reproducibly

67d6510

If BFBFLAG is set (e.g., by the ERP, PEM, PET and SEQ tests, or by the user), then compute these global sums in a way that is bit-for-bit independent of processor count.

Write smb renormalization stats with full precision

422d4ef

Move bfbflag to ALLCOMP_attributes

e58dd4c

We'll need this in CDEPS (DGLC) as well

Move global-sum routine to med_utils_mod

c1960de

Use med_global_sums in med_phases_prep_atm_enthalpy_correction

5fe5dc7

Move med_global_sums to its own module

2fc288b

The motivation for this is so that we have nearly-identical modules between CMEPS and CDEPS for ease of diffability.

Use med_global_sums for global sums in post_rof_mod

bdfc64e

This allows these global sums to be reproducible with different processor counts if bfbflag is set. Code changes made by Cladue, reviewed carefully by myself.

Merge tag 'cmeps1.1.68' into glc_renormalize_smb_reprosum

d828c22

cmeps1.1.68

Merge tag 'cmeps1.1.69' into glc_renormalize_smb_reprosum

74dbb60

cmeps1.1.69

billsacks requested a review from Katetc September 17, 2026 16:02

Fix unused variable warning

b022c95

Katetc reviewed Sep 17, 2026

View reviewed changes

Improve bfbflag description from PR review

e151743

Katetc approved these changes Sep 17, 2026

View reviewed changes

Better wording in comment

0b39c0a

Delete a comment that is more confusing than helpful

b674904

billsacks merged commit 0060d41 into ESCOMP:main Sep 18, 2026
1 check passed

billsacks deleted the glc_renormalize_smb_reprosum branch September 18, 2026 19:51

billsacks mentioned this pull request Sep 21, 2026

merge noresm cmeps1.1.57_noresm_v0 #690

Merged

mvertens mentioned this pull request Sep 21, 2026

merge to cmeps1.1.71 NorESMhub/CMEPS#62

Merged


		rc = ESMF_SUCCESS

		allocate(summands_2d(size(local_summands), 1))

Conversation

billsacks commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Specific notes

Testing performed

Uh oh!

billsacks commented Sep 17, 2026

Uh oh!

Katetc commented Sep 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billsacks commented Sep 17, 2026

Uh oh!

Katetc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billsacks commented Sep 17, 2026

Uh oh!

billsacks commented Sep 18, 2026

Uh oh!

billsacks commented Sep 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

billsacks commented Sep 17, 2026 •

edited

Loading