Main candidate from NCAR (04/21/2021) by gustavo-marques · Pull Request #1381 · mom-ocean/MOM6

gustavo-marques · 2021-04-21T19:59:30Z

Summary

Many improvements in the lateral boundary diffusion module
Adds a new counter-based random number generator
Fixes a minor bug in the neutral diffusion module (only applies when NDIFF_INTERIOR_ONLY = True)
Fixes sign bug in the latent heat contribution from frozen precipitation and frozen runoff (MCT and NUOPC caps)
Adds option to apply an are flux correction in the NUOPC cap
Adds support for threading in the NUOPC cap

This PR does not change the MOM6-examples answers (w.r.t. branch main at 1a420f2) when using the intel/19.0.5 compiler. It also does not add new MOM input flags or diagnostics.

…n_code_LBD Delete forgotten code

* Also reducing line lenghts > 120

Fixes inconsistency in the calculation of tendency due to lateral diffusion

See https://arxiv.org/abs/2004.06278. Not an exact reproduction of "squares" because Fortran doesn't have a uint64 type, and not all compilers provide integers with > 64 bits...

…_diff Adds mask2dT checker to avoid calling boundary_k_range at land points

Added a counter-based PRNG to MOM_random

Remove an omp parallel do directive due to thread-unsafe cpu_clock

This patch fixes a sign bug, in both MCT and NUOPC, when accounting for the latent heat from fprec and frunnoff. Following MOM6's definition, both fprec and frunoff are > 0 into the ocean. Therefore, the latent heat associated with these terms should be negative.

* Fixes latent heat contribution from fprec and frunoff (MCT and NUOPC)

…dev/ncar

…2021-02-19

…ndidate_2021-02-19 Fix LBD module after merging main candidate 2021 02 19

Merge main candidate 2021-02-19

Add med2mod_areacor and move where area correction is applied

Sync with main to bring in new file layout

gustavo-marques · 2021-04-21T20:26:25Z

@kshedstrom, @jiandewang, @abozec (or @awallcraft): please review/test this PR.

jiandewang · 2021-04-21T20:43:15Z

@gustavo-marques https://github.com/NOAA-GFDL/MOM6/pull/1381/files#diff-68076a5015442fbab97fced84c6d2c139c715798d26b7e0fe0c542bab0b3c1d6R467-R478
use function ChkErr(rc,LINE,u_FILE_u) to replace
if (ESMF_LogFoundError

gustavo-marques · 2021-04-21T20:58:17Z

@jiandewang, thanks for catching that.

kshedstrom · 2021-04-22T04:59:19Z

I approve this PR.

abozec

COAPS approves this PR.

DeniseWorthen · 2021-04-22T21:17:05Z

+  call ESMF_VMGet(vm, pet=localPet, peCount=localPeCount, rc=rc)
+  if (ChkErr(rc,__LINE__,u_FILE_u)) return
+
+  if(localPeCount == 1) then


The two instances of retrieving the attribute 'nthreads' are failing for EMC. They will need "isPresent,isSet" conditionals added. Thanks.

Done. Thanks, @DeniseWorthen.

DeniseWorthen · 2021-04-26T12:32:59Z

@gustavo-marques This still isn't working for EMC. We need to use the isPresent,isSet conditionals to optionally retrieve the config variable. I've tested the following change for our configuration on Gaea:

Please check the logic and make sure it is what you intend. Thanks.

diff --git a/config_src/drivers/nuopc_cap/mom_cap.F90 b/config_src/drivers/nuopc_cap/mom_cap.F90
index f5f7985e1..742b7575d 100644
--- a/config_src/drivers/nuopc_cap/mom_cap.F90
+++ b/config_src/drivers/nuopc_cap/mom_cap.F90
@@ -418,6 +418,7 @@ subroutine InitializeAdvertise(gcomp, importState, exportState, clock, rc)
   character(len=512)                     :: diro
   character(len=512)                     :: logfile
   character(ESMF_MAXSTR)                 :: cvalue
+  character(len=64)                      :: logmsg
   logical                                :: isPresent, isPresentDiro, isPresentLogfile, isSet
   logical                                :: existflag
   integer                                :: userRc
@@ -467,13 +468,19 @@ subroutine InitializeAdvertise(gcomp, importState, exportState, clock, rc)
   if (ChkErr(rc,__LINE__,u_FILE_u)) return

   if(localPeCount == 1) then
-     call NUOPC_CompAttributeGet(gcomp, "nthreads", value=cvalue, &
+     call NUOPC_CompAttributeGet(gcomp, name="nthreads", value=cvalue, &
           isPresent=isPresent, isSet=isSet, rc=rc)
      if (ChkErr(rc,__LINE__,u_FILE_u)) return
-     read(cvalue,*) nthrds
+     if (isPresent .and. isSet) then
+        read(cvalue,*) nthrds
+     else
+        nthrds = localPeCount
+     end if
   else
      nthrds = localPeCount
   endif
+  write(logmsg,*) nthrds
+  call ESMF_LogWrite(trim(subname)//': nthreads = '//trim(logmsg), ESMF_LOGMSG_INFO)

 !$  call omp_set_num_threads(nthrds)

@@ -898,10 +905,14 @@ subroutine InitializeRealize(gcomp, importState, exportState, clock, rc)
   if (ChkErr(rc,__LINE__,u_FILE_u)) return

   if(localPeCount == 1) then
-     call NUOPC_CompAttributeGet(gcomp, "nthreads", value=cvalue, &
+     call NUOPC_CompAttributeGet(gcomp, name="nthreads", value=cvalue, &
           isPresent=isPresent, isSet=isSet, rc=rc)
      if (ChkErr(rc,__LINE__,u_FILE_u)) return
-     read(cvalue,*) nthrds
+     if (isPresent .and. isSet) then
+        read(cvalue,*) nthrds
+     else
+        nthrds = localPeCount
+     end if
   else
      nthrds = localPeCount
   endif

gustavo-marques · 2021-04-26T17:40:23Z

@DeniseWorthen, sorry for breaking your tests, and thank you for suggesting a solution. I will add your changes and run them by Jim Edwards since he implemented the threading option in NUOPC.

jiandewang · 2021-04-26T22:18:41Z

@gustavo-marques I will run UFS RT with your updated code, but may take some time as we ran out of CPU quota at this moment, job turnover rate is extremely low.

jiandewang · 2021-04-28T16:57:51Z

@gustavo-marques: by adding !$ call omp_set_num_threads(nthrds) in the cap code, does it mean part of cap will be running with OMP or something else ? we don't have a clear idea here, can you explain ?

The updated code passed UFS RT, thanks for @DeniseWorthen and @gustavo-marques

alperaltuntas · 2021-04-28T17:51:28Z

@jiandewang, these (or any) OpenMP directives are effective only if you explicitly enable OpenMP at compile time. Unless you compile with OpenMP, these directives (!$) are merely regarded as comments, and so will not affect your model. If you are compiling with OpenMP, on the other hand, the directive you refer to does the following: "Instead of using the default number of threads, the directive sets the number of threads to the value received from NUOPC_CompAttributeGet call.

Also mentioned @jedwards4b, if he likes to add more.

jiandewang · 2021-04-28T19:53:42Z

@jiandewang, these (or any) OpenMP directives are effective only if you explicitly enable OpenMP at compile time. Unless you compile with OpenMP, these directives (!$) are merely regarded as comments, and so will not affect your model. If you are compiling with OpenMP, on the other hand, the directive you refer to does the following: "Instead of using the default number of threads, the directive sets the number of threads to the value received from NUOPC_CompAttributeGet call.

Also mentioned @jedwards4b, if he likes to add more.

@alperaltuntas thanks for the explanation, did you get identical results from different threading # ?

marshallward · 2021-04-28T20:28:08Z

GFDL Gaea regression (internal): https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/12566 ✔️

alperaltuntas · 2021-04-29T02:15:57Z

@jiandewang Yes, the number of threads doesn't change the results.

marshallward · 2021-05-03T16:14:40Z

@sanAkel Did you want to review this PR? The number of non-NUOPC changes is very small so I would not expect any issues.

Otherwise, I will go ahead and merge this if there's no objection. (I will redirect it to main rather than dev/gfdl).

sanAkel · 2021-05-03T17:31:47Z

@sanAkel Did you want to review this PR? The number of non-NUOPC changes is very small so I would not expect any issues.

Otherwise, I will go ahead and merge this if there's no objection. (I will redirect it to main rather than dev/gfdl).

@marshallward Thanks for checking. I think I will take a pass on this one. Though I am a bit confused by "(I will redirect it to main rather than dev/gfdl)" You won't merge it to dev/gfdl ?

marshallward · 2021-05-03T18:00:17Z

I am a bit confused by "(I will redirect it to main rather than dev/gfdl)" You won't merge it to dev/gfdl ?

The PR was misdirected to the dev/gfdl branch, but we want it to go to main, i.e. our release branch. I will manually merge this to main but will later merge the contents into dev/gfdl.

sanAkel · 2021-05-03T18:34:14Z

I am a bit confused by "(I will redirect it to main rather than dev/gfdl)" You won't merge it to dev/gfdl ?

The PR was misdirected to the dev/gfdl branch, but we want it to go to main, i.e. our release branch. I will manually merge this to main but will later merge the contents into dev/gfdl.

Got it, Thanks @marshallward. Once you have

later merge the contents into dev/gfdl.

We will update GEOS-ESM.

marshallward · 2021-05-03T19:15:11Z

This PR has now been added to main. It should auto-close once we merge into dev/gfdl.

gustavo-marques and others added 30 commits December 15, 2020 18:13

Merge pull request mom-ocean#169 from gustavo-marques/delete_fotgotte…

0361c2e

…n_code_LBD Delete forgotten code

Fixes inconsistency in the calculation of tendency

170b4be

* Also reducing line lenghts > 120

Calculate tendency using fluxes

99dcfbb

Merge pull request mom-ocean#170 from gustavo-marques/fix_lbd_tendency

49c0167

Fixes inconsistency in the calculation of tendency due to lateral diffusion

Added a counter-based PRNG to MOM_random

280e98b

See https://arxiv.org/abs/2004.06278. Not an exact reproduction of "squares" because Fortran doesn't have a uint64 type, and not all compilers provide integers with > 64 bits...

Used int64 from F2003 and added doc strings

62138be

Avoids calling boundary_k_range at land pts

dde80ba

Merge branch 'pr172' into dev/ncar

20a35db

Merge pull request mom-ocean#172 from gustavo-marques/fix_bug_neutral…

28d791e

…_diff Adds mask2dT checker to avoid calling boundary_k_range at land points

Merge pull request mom-ocean#171 from iangrooms/dev/ncar

1c8a810

Added a counter-based PRNG to MOM_random

remove omp parallel do directive for a do loop including cpu_clock calls

0262e93

Merge pull request mom-ocean#173 from NCAR/fix_omp_set_diff

cc2b999

Remove an omp parallel do directive due to thread-unsafe cpu_clock

Merge branch 'pr174' into dev/ncar

5837db6

Merge pull request mom-ocean#174 from gustavo-marques/latent_heat_fix

230b839

* Fixes latent heat contribution from fprec and frunoff (MCT and NUOPC)

incorporated flux correction factors

e209c0e

Merge branch 'dev/ncar' of https://github.com/NCAR/MOM6 into dev/ncar

ed58876

resolve minor conflicts while merging main candidate 2021-02-19 into …

b85db47

…dev/ncar

Merge branch 'pr1328_2' into main_candidate_2021-02-19

d29db6c

reduce the line length in mom_surface_forcing_mct

387166b

Add missing areacor

1e2fbf4

Fix LBD module after merging main_candidate_2021-02-19

87298d7

Merge branch 'main_candidate_2021-02-19' into fix_lbd_main_candidate_…

0252b7c

…2021-02-19

Merge pull request mom-ocean#178 from gustavo-marques/fix_lbd_main_ca…

a6cdca9

…ndidate_2021-02-19 Fix LBD module after merging main candidate 2021 02 19

Merge pull request mom-ocean#177 from NCAR/main_candidate_2021-02-19

9ced10b

Merge main candidate 2021-02-19

Remove unused imports and delete blank line

5667b70

Merge pull request mom-ocean#179 from gustavo-marques/flux_areacorr_eval

e97d229

Add med2mod_areacor and move where area correction is applied

Merge branch 'pr1342' into merge_new_file_layout

d8c0122

Merge remote-tracking branch 'gfdl/main' into merge_new_file_layout

e87dffb

Merge pull request mom-ocean#181 from NCAR/merge_new_file_layout

80cb48e

Sync with main to bring in new file layout

Merge branch 'main' into main-candidate-ncar-2021-04-21

0a34093

gustavo-marques changed the title ~~Main candidate from NCAR (2021/04/21)~~ Main candidate from NCAR (04/21/2021) Apr 21, 2021

Replaces ESMF_LogFoundError to ChkErr

3b121cf

abozec approved these changes Apr 22, 2021

View reviewed changes

DeniseWorthen reviewed Apr 22, 2021

View reviewed changes

Add isPresent and isSet when retrieving nthreads

6196c03

use isPresent,isSet to conditionally retrieve configuration

e6ce6a8

marshallward merged commit 061100e into mom-ocean:dev/gfdl May 3, 2021

This was referenced May 4, 2021

update MOM6 to GFDL's 20210503 commit NOAA-EMC/MOM6#66

Closed

Feature/update to gfdl 20210503 NOAA-EMC/MOM6#67

Merged

update MOM6 to GFDL's 20210503 commit ufs-community/ufs-weather-model#556

Closed

marshallward mentioned this pull request Jul 8, 2021

Dev gfdl main candidate 2021 07 08 #1440

Merged

gustavo-marques deleted the main-candidate-ncar-2021-04-21 branch February 25, 2022 15:35

Conversation

gustavo-marques commented Apr 21, 2021

Summary

Uh oh!

gustavo-marques commented Apr 21, 2021

Uh oh!

jiandewang commented Apr 21, 2021

Uh oh!

gustavo-marques commented Apr 21, 2021

Uh oh!

kshedstrom commented Apr 22, 2021

Uh oh!

abozec left a comment

Choose a reason for hiding this comment

Uh oh!

DeniseWorthen Apr 22, 2021

Choose a reason for hiding this comment

Uh oh!

gustavo-marques Apr 22, 2021

Choose a reason for hiding this comment

Uh oh!

DeniseWorthen commented Apr 26, 2021

Uh oh!

gustavo-marques commented Apr 26, 2021

Uh oh!

jiandewang commented Apr 26, 2021

Uh oh!

jiandewang commented Apr 28, 2021

Uh oh!

alperaltuntas commented Apr 28, 2021

Uh oh!

jiandewang commented Apr 28, 2021

Uh oh!

marshallward commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alperaltuntas commented Apr 29, 2021

Uh oh!

marshallward commented May 3, 2021

Uh oh!

sanAkel commented May 3, 2021

Uh oh!

marshallward commented May 3, 2021

Uh oh!

sanAkel commented May 3, 2021

Uh oh!

marshallward commented May 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

marshallward commented Apr 28, 2021 •

edited

Loading