replace global sum with full-precision reproducible sum #6456

maltrud · 2024-05-31T16:00:04Z

The global sum in the surface salinity restoring routine was forced to be BFB using a truncation calculation that resulted in reduced precision and some unexplained behavior. This PR replaces this calculation with an mpas standard reproducible sum with full precision.

Testing was done using a G-case with the IcoswISC30E3r5 grid for one year, then calculating the global average SSS restoring tendency term, which should be effectively zero. Also confirmed that the new code resulted in BFB solution when processor layout was changed from 10 nodes on chrysalis to 15 nodes.

Fixes #6446
[non-BFB] for C- and G-cases with salinity restoring

github-actions · 2024-05-31T16:02:24Z

PR Preview Action v1.4.7
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6456/
on branch `gh-pages` at 2024-06-20 21:03 UTC

maltrud · 2024-05-31T16:05:14Z

Here's a time series of the log10 of the absolute value of the globally averaged SSS tendency (monthly averages) for 4 simulations.

The black line is the old global sum with real4 output, and the green is the old sum with real8 output. The red line is the new reproducible sum with r4 output, and the blue is the new sum with real8 output.

vanroekel · 2024-06-03T20:12:48Z

components/mpas-ocean/src/shared/mpas_ocn_tracer_surface_restoring.F

@@ -414,11 +422,14 @@ subroutine ocn_get_surfaceSalinityData( streamManager, &

        endif

+        indexForReproSum(1) = 1
+        indexForReproSum(2) = 0


This is probably just do to the construction of the repro sum, but I'm a bit confused by the two length array of indexForReproSum. It seems position 1 is always just 1 and position 2 is the total number of entries in the sum. Is that right? What is position 1 of this index array used for? It never appears to be modified and then just get used in the globalSum call.

@vanroekel The reproducible sums in MPAS allow sums over full arrays and over restricted address ranges in each dimension. For full generality, the range must be specified in min/max pairs for each array dimension. In this case, the array is getting packed in advance so the min is always 1 but the max can vary. But both need to be provided for the interface.

Thanks @philipwjones for the explanation. It didn't make sense to me either. Can we add a comment to explain that, if it's cleaner to retain the indexForReproSum with dimensions nSums and indexForReproSum(1) = min = 1 ?

@vanroekel this has always been part of the algorithm (essentially the "if deltaS .ne. 0.0" part) to account for possible cases where we explicitly don't want to apply restoring to some cells (eg, under land ice maybe?). then we don't actually want to include every cell in these sums. does that make sense?

Suggested change

indexForReproSum(2) = 0

indexForReproSum(2) = 0

! For reproducible sums like mpas_global_sum_mod(), the range must be specified in min/max pairs for each array dimension.

! In this case, the array is getting packed in advance so the min is always 1 but the max can vary.

! Both need to be provided for the interface.

Thanks @philipwjones for the help understanding this. I'd like to include the comment from @alicebarthel in the PR for clarity. If it is too long for a comment, perhaps we can think of a shorter version.

alicebarthel · 2024-06-03T21:46:57Z

components/mpas-ocean/src/shared/mpas_ocn_tracer_surface_restoring.F

-        real (kind=RKIND), dimension(nSums) :: reductions, sums
+        real (kind=RKIND), dimension(nSums) :: reductions
+
+        integer, dimension(nSums) :: indexForReproSum


@philipwjones It's a minor detail, but I'm curious. Can you confirm if I understood this correctly: in this case, it's a coincidence that indexForReproSum and reductions have the same dimensions of nSums?
Technically, indexForReproSum should always have 2 (the min, max you mentioned), while nSums could be anything given the needs of the routine? (here, 2 since we need the area sum and the area-weighted salinity sum).
Does it make sense to use nSums to define indexForReproSum then? It seems misleading to me.

@alicebarthel Good catch. The index array should be dimensioned 2*(number of dimensions being summed over) - in this case just 2 since the arrays are just 1-d. So nSums is coincidentally correct but in incorrect in general.

@alicebarthel I wouldn't say it is "a coincidence" that nSums=2; this routine isn't meant to be general which is why I hardwired it to be correct for this case.

@maltrud sure, I understand it's not meant to be general. I don't mind hard-coding the dimensions of indexForReproSum to 2. I mind hard-coding it to nSums if it's not supposed to have nSums as a dimension in general.

ok, maybe I still don't understand the question. we have 2 sums to do: area and areadeltaS, so nSums = 2. so we need the same number of outputs from the 2 global sum calls: global_sum(area) and global_sum(areadeltaS) which are put into reductions(1) and reductions(2), respectively. so reductions should always be dimensioned nSums. I guess I could have just put sumAreaGlobal and sumAreaDeltaSGlobal directly in the global sum calls and not needed reductions(), but I wanted to keep the new version as similar to the current version as possible. does this make sense?

ah, ok, now I understand. you're right, that is misleading. should I just place it with "2"?

and a comment? thanks for clarifying.

I think you should create a variable ThroatWarblerMangrove that's pronounced "two"...

@maltrud Sounds good to me, thanks!

Could I suggest Rhabarberbarbarabarbarbarenbartbarbierbier as a suitable alternative?

alicebarthel · 2024-06-03T21:48:33Z

components/mpas-ocean/src/shared/mpas_ocn_tracer_surface_restoring.F

@@ -250,7 +251,11 @@ subroutine ocn_get_surfaceSalinityData( streamManager, &
        integer :: ierr

        integer, parameter :: nSums = 2
-        real (kind=RKIND), dimension(nSums) :: reductions, sums
+        real (kind=RKIND), dimension(nSums) :: reductions


@maltrud just curious: is there any reason you went with reductions as a name for these variables? Sums is more intuitive to me, but maybe I'm not familiar with the jargon?

@alicebarthel I wanted to make as few code changes as possible so I retained the same variable names that already existed whenever possible. in the new code, "sums" isn't needed anymore, while "reductions" serves the same purpose in both the current and new versions. probably should have done things differently many years ago, but that's the rationale.

thanks for your careful reviewing.

@maltrud I totally understand the wisdom of making minimal code changes. As a newcomer to these routines, I allow myself to make minor comments as to readability to new users. Take it or leave it :-)

alicebarthel

Approving based on visual inspection, provided that change to L256 is made.

mark-petersen

I compiled this with gnu (debug and optimized) on chicoma and intel on chrysalis and tested with the nightly test suite. That tests the compiling, but not the function of this code.

Thanks for running the G case tests. That is convincing. Since you are able to show reproducible sums on different processor counts, I'm happy to have this merged in.

maltrud · 2024-06-20T21:03:09Z

I've made the recommended changes and push changes to the branch.

jonbob · 2024-06-24T18:10:26Z

@vanroekel -- do you approve now with the changes?

vanroekel

Approving based on visual inspection and testing from @maltrud and @alicebarthel

Replace global sum with full-precision reproducible sum The global sum in the surface salinity restoring routine was forced to be BFB using a truncation calculation that resulted in reduced precision and some unexplained behavior. This PR replaces this calculation with an mpas standard reproducible sum with full precision. Testing was done using a G-case with the IcoswISC30E3r5 grid for one year, then calculating the global average SSS restoring tendency term, which should be effectively zero. Also confirmed that the new code resulted in BFB solution when processor layout was changed from 10 nodes on chrysalis to 15 nodes. Fixes #6446 [non-BFB] for C- and G-cases with salinity restoring

jonbob · 2024-07-09T19:14:10Z

As expected, passes:

ERP_Ld3.ne30pg2_r05_IcoswISC30E3r5.WCYCL1850.chrysalis_intel.allactive-pioroot1
PET_Ln5.T62_oQU240wLI.GMPAS-DIB-IAF-DISMF.chrysalis_intel (G-case but no restoring file)

and DIFFs for:

SMS_D_Ld1.TL319_IcoswISC30E3r5.GMPAS-JRA1p5-DIB-PISMF.chrysalis_intel.mpaso-jra_1958 (G-case with restoring file)

merged to next

jonbob · 2024-07-10T16:09:25Z

merged to master and expected DIFFs blessed

This merge updates the E3SM-Project submodule from [c7d7998](https://github.com/E3SM-Project/E3SM/tree/c7d7998) to [727ad81](https://github.com/E3SM-Project/E3SM/tree/727ad81). This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list): - [ ] (ocn) E3SM-Project/E3SM#6456 - [ ] (ocn) E3SM-Project/E3SM#6510 - [ ] (ocn) E3SM-Project/E3SM#6535

replace global sum with full-precision reproducible sum

62fcccf

maltrud added mpas-ocean non-BFB PR makes roundoff changes to answers. labels May 31, 2024

maltrud assigned jonbob May 31, 2024

rljacob requested a review from alicebarthel May 31, 2024 16:07

jonbob requested a review from vanroekel May 31, 2024 16:34

jonbob added the bug fix PR label May 31, 2024

vanroekel reviewed Jun 3, 2024

View reviewed changes

alicebarthel reviewed Jun 3, 2024

View reviewed changes

alicebarthel approved these changes Jun 9, 2024

View reviewed changes

mark-petersen self-requested a review June 20, 2024 17:24

mark-petersen approved these changes Jun 20, 2024

View reviewed changes

changed confusing dim on indexForReproSum, added comments

7ca7088

vanroekel approved these changes Jul 9, 2024

View reviewed changes

jonbob merged commit 429dc36 into master Jul 10, 2024
13 checks passed

jonbob deleted the maltrud/ocean/sss-restoring-reprosum branch July 10, 2024 15:51

xylar mentioned this pull request Aug 8, 2024

Update the E3SM-Project submodule MPAS-Dev/compass#849

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace global sum with full-precision reproducible sum #6456

replace global sum with full-precision reproducible sum #6456

maltrud commented May 31, 2024 •

edited by jonbob

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

maltrud commented May 31, 2024

vanroekel Jun 3, 2024

philipwjones Jun 3, 2024

alicebarthel Jun 3, 2024 •

edited

Loading

maltrud Jun 3, 2024

alicebarthel Jun 3, 2024 •

edited

Loading

vanroekel Jun 3, 2024

alicebarthel Jun 3, 2024 •

edited

Loading

philipwjones Jun 3, 2024

maltrud Jun 4, 2024

alicebarthel Jun 4, 2024 •

edited

Loading

maltrud Jun 4, 2024

maltrud Jun 4, 2024

maltrud Jun 4, 2024

philipwjones Jun 4, 2024

alicebarthel Jun 4, 2024

xylar Jun 4, 2024

alicebarthel Jun 3, 2024

maltrud Jun 4, 2024

alicebarthel Jun 4, 2024

alicebarthel left a comment •

edited

Loading

mark-petersen left a comment

maltrud commented Jun 20, 2024

jonbob commented Jun 24, 2024

vanroekel left a comment

jonbob commented Jul 9, 2024

jonbob commented Jul 10, 2024

-        indexForReproSum(2) = 0
+        indexForReproSum(2) = 0
+      ! For reproducible sums like mpas_global_sum_mod(), the range must be specified in min/max pairs for each array dimension.
+      ! In this case, the array is getting packed in advance so the min is always 1 but the max can vary.
+      ! Both need to be provided for the interface.

replace global sum with full-precision reproducible sum #6456

replace global sum with full-precision reproducible sum #6456

Conversation

maltrud commented May 31, 2024 • edited by jonbob Loading

github-actions bot commented May 31, 2024 • edited Loading

maltrud commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicebarthel Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicebarthel Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicebarthel Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicebarthel Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicebarthel left a comment • edited Loading

Choose a reason for hiding this comment

mark-petersen left a comment

Choose a reason for hiding this comment

maltrud commented Jun 20, 2024

jonbob commented Jun 24, 2024

vanroekel left a comment

Choose a reason for hiding this comment

jonbob commented Jul 9, 2024

jonbob commented Jul 10, 2024

maltrud commented May 31, 2024 •

edited by jonbob

Loading

github-actions bot commented May 31, 2024 •

edited

Loading

alicebarthel Jun 3, 2024 •

edited

Loading

alicebarthel Jun 3, 2024 •

edited

Loading

alicebarthel Jun 3, 2024 •

edited

Loading

alicebarthel Jun 4, 2024 •

edited

Loading

alicebarthel left a comment •

edited

Loading