Fix uninitialized min_rand variable in Thompson MP when using SPP #892

JeffBeck-NOAA · 2022-04-02T18:29:33Z

Description

This suite of PRs fixes the uninitialized min_rand variable in Thompson MP when using SPP and is explained in detail in the ufs-weather-model PR here.

Issue(s) addressed

Fixes NCAR/ccpp-physics Issue #891.

Testing

The RT suite on Hera passed for all normal and debug tests, except for the regional_spp_sppt_shum_skeb RT test, which will now have a new baseline result.

@jwolff-ncar, @willmayfield, @bluefinweiwei, @michelleharrold, @judithberner, @gsketefian

gthompsnWRF

Kinda important to catch the minimum value of rand_pert. Approved.

SamuelTrahanNOAA

This PR will not have consistent results between thread counts due to this line in module_mp_thompson.F90

abs_min_rand = ABS(MINVAL(rand_pert(:,1)))

SamuelTrahanNOAA · 2022-04-04T20:12:33Z

@grantfirl

Kinda important to catch the minimum value of rand_pert. Approved.

It doesn't solve the problem correctly though, since it is only getting the minimum of the current block, not the whole domain. The results will vary depending on threading and blocking settings.

climbfuji · 2022-04-04T20:14:42Z

Excellent catch, Sam.

…

On Apr 4, 2022, at 2:12 PM, Samuel Trahan (NOAA contractor) ***@***.***> wrote: @grantfirl <https://github.com/grantfirl> Kinda important to catch the minimum value of rand_pert. Approved. It doesn't solve the problem correctly though, since it is only getting the minimum of the current block, not the whole domain. The results will vary depending on threading and blocking settings. — Reply to this email directly, view it on GitHub <#892 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C2RIEVMQY57BJ3JBL2S3VDNEL3ANCNFSM5SLIWTRQ>. You are receiving this because your review was requested.

SamuelTrahanNOAA · 2022-04-04T20:15:00Z

The random fields are generally normalized somehow, right? Perhaps [-1,1] or [0,1]. If you know how it was normalized, then you can use that value (-1 or 0 in those examples) instead of computing them.

gthompsnWRF · 2022-04-04T20:21:05Z

The random fields are generally normalized somehow, right? Perhaps [-1,1] or [0,1]. If you know how it was normalized, then you can use that value (-1 or 0 in those examples) instead of computing them.

I was going to ask about thread safe, but I just don't know how CCPP code is structured as well as I do WRF where I had implemented this in a thread-safe way.

Yes, actually I believe the min value should always be set to -1.0, so in this case, I think it is actually best to set abs_min_rand as a universal constant of 1.0. I realize now how this could be a flaw in my original design because instead of always asking for the lowest possible negative number to add to the original number, I was doing the abs of lowest number at the time, so in the case of slow spin-up, the min_rand number is hardly ever as low as -1.0. So I now believe this can be changed to a simple constant of 1.0 (which would be abs(-1.0) basically.

JeffBeck-NOAA · 2022-04-04T20:31:44Z

All blocks are consolidated into a single 1D array in the stochastic physics wrapper (stochastic_physics_wrapper.F90) before spp_wts_mp is passed to module_mp_thompson.F90 as rand_pert:

do nb=1,Atm_block%nblks
      GFS_Data(nb)%Coupling%spp_wts_mp(:,:) = spp_wts(nb,1:GFS_Control%blksz(nb),:,n)
end do

SamuelTrahanNOAA · 2022-04-04T20:33:22Z

It will still depend on MPI decomposition (layout_x and layout_y) since different ranks will have different parts of the domain.

JeffBeck-NOAA · 2022-04-04T20:33:38Z

The random fields are generally normalized somehow, right? Perhaps [-1,1] or [0,1]. If you know how it was normalized, then you can use that value (-1 or 0 in those examples) instead of computing them.

The random fields aren't normalized, but are capped based on namelist settings (magnitude*cutoff), so they will vary from run to run.

JeffBeck-NOAA · 2022-04-04T20:36:23Z

The random fields are generally normalized somehow, right? Perhaps [-1,1] or [0,1]. If you know how it was normalized, then you can use that value (-1 or 0 in those examples) instead of computing them.

The ramdon fields aren't normalized, but are capped based on a namelist setting, so they will vary from run to run.

It will still depend on MPI decomposition (layout_x and layout_y) since different ranks will have different parts of the domain.

OK, that makes sense now. It looks like only a constant will work in this case.

SamuelTrahanNOAA · 2022-04-04T20:37:10Z

The only way I see of doing this is an MPI_Allreduce at the model level, and pass the result down to CCPP. It is likely the stochastic physics component already does that calculation, but it may not send the minimum to the model.

gthompsnWRF · 2022-04-04T20:38:36Z

Please waste no more energy on this and consider setting this as constant since I think it is the right thing to do. If Jeff has other thoughts, please speak out.

climbfuji · 2022-04-04T20:39:07Z

You can do an MPI exchange in the time_vary group (timestep_init phase in CCPP lingo).

…

On Apr 4, 2022, at 2:37 PM, Samuel Trahan (NOAA contractor) ***@***.***> wrote: The only way I see of doing this is an MPI_Allreduce at the model level, and pass the result down to CCPP. It is likely the stochastic physics component already does that calculation, but it may not send the minimum to the model. — Reply to this email directly, view it on GitHub <#892 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5C2RO3JSIFFU4O4WTFRP3VDNHIDANCNFSM5SLIWTRQ>. You are receiving this because your review was requested.

JeffBeck-NOAA · 2022-04-04T20:41:28Z

Are the following variables (rand1 and rand2) non-reproducable as well? It seems like if rand_pert(:,1) won't work, rand_pert(i,1) won't work either?

if (rand_perturb_on .ne. 0) then
     if (MOD(rand_perturb_on,2) .ne. 0) rand1 = rand_pert(i,1)
     m = RSHIFT(ABS(rand_perturb_on),1)
     if (MOD(m,2) .ne. 0) rand2 = rand_pert(i,1)*2.
     m = RSHIFT(ABS(rand_perturb_on),2)
     if (MOD(m,2) .ne. 0) rand3 = 0.25*(rand_pert(i,1)+abs_min_rand)
     m = RSHIFT(ABS(rand_perturb_on),3)
endif

SamuelTrahanNOAA · 2022-04-04T20:43:36Z

Are the following variables (rand1 and rand2) non-reproducable as well?

No. Those are in an i,j loop, so rand1 is a temporary variable containing data at that i,j point. (Likewise with rand2 and rand3).

SamuelTrahanNOAA · 2022-04-04T21:05:07Z

@gthompsnWRF If you want to have a meeting, please contact your desired attendees by email, and we'll arrange something. I'd rather not discuss our schedules and locations on github. Joe Olson should be there too, since his PR is the one that sparked all this.

gthompsnWRF · 2022-04-05T15:53:50Z

Jeff: yes, this is the best possible solution! I forgot that the random field is not intended to be [-1,1]. This solution is the proper one for certain.

JeffBeck-NOAA · 2022-04-05T17:25:13Z

Jeff: yes, this is the best possible solution! I forgot that the random field is not intended to be [-1,1]. This solution is the proper one for certain.

Thanks, Greg! It doesn't look like it will be too difficult to bring these namelist entries into CCPP (two of the three are already available), so I should have it ready in a day or so. Thanks to @grantfirl for his help here.

gthompsnWRF · 2022-04-06T02:34:48Z

physics/module_mp_thompson.F90

@@ -1027,7 +1028,8 @@ SUBROUTINE mp_gt_driver(qv, qc, qr, qi, qs, qg, ni, nr, nc,     &
                          re_cloud, re_ice, re_snow
      INTEGER, INTENT(IN) :: rand_perturb_on, kme_stoch
      REAL, DIMENSION(:,:), INTENT(IN) :: &
-                          rand_pert
+                          rand_pert,spp_prt_list,spp_stddev_cutoff,n_var_spp, &


I believe rand_pert is a 2D var, but how are these namelist variables dimensioned as (:,:) instead of 1-D? Shouldn't these additions be 1-D or at least separated from the (i,j) assumption of the rand_pert variable? Also, I do not find prt_list as intuitive. My first guess of this variable name would be something like "print_list" so is there anything more intuitive?

@gthompsnWRF, none of this code is really ready to be reviewed yet, as I was just pushing things to my fork to then clone ufs-weather-model and run the stochastic physics RT. I probably should have kept all changes local until I got done with this work. Sorry for the confusion. That said, thanks for finding an error in the declarations here, those fields are 1D, and shouldn't go in this 2D line.

gthompsnWRF · 2022-04-06T02:37:43Z

physics/module_mp_thompson.F90

@@ -1101,7 +1103,7 @@ SUBROUTINE mp_gt_driver(qv, qc, qr, qi, qs, qg, ni, nr, nc,     &
      REAL, DIMENSION(its:ite, jts:jte):: pcp_ra, pcp_sn, pcp_gr, pcp_ic
      REAL:: dt, pptrain, pptsnow, pptgraul, pptice
      REAL:: qc_max, qr_max, qs_max, qi_max, qg_max, ni_max, nr_max
-      REAL:: rand1, rand2, rand3, min_rand
+      REAL:: rand1, rand2, rand3, spp_mp_mag_times_cutoff


Couldn't this be made a simpler (and shorter) and intuitive name like: rand_pert_max?

Yes, I was trying to keep some provenance for the source of this field, but it can be found in the routine itself (spp_prt_list(k)*spp_stddev_cutoff(k)), and rand_pert_max is a better idea.

gthompsnWRF · 2022-04-06T02:41:16Z

physics/module_mp_thompson.F90

+        do k =1,n_var_spp
+          select case (spp_var_list(k))
+          case('mp')
+            spp_mp_mag_times_cutoff = spp_prt_list(k)*spp_stddev_cutoff(k)


There is no else clause here, so is it safe to have this variable without being initialized somewhere? Can it safely be set to zero (or perhaps did I miss it getting a default value)?

Yes, I still need to initialize the variable. I think setting it to zero will be fine (it's not used in that case anyway).

gthompsnWRF · 2022-04-06T02:41:35Z

physics/module_mp_thompson.F90

+        enddo
+      endif
+
+      print*, ' spp_mp_mag_times_cutoff is = ', spp_mp_mag_times_cutoff


A print statement like this should be reserved for debugging only not in production code.

Yes, this was just for my testing purposes with this intermediate code. I will remove it once I confirm things are getting passed into the routine and applied correctly.

JeffBeck-NOAA · 2022-04-06T22:19:36Z

@gthompsnWRF, @SamuelTrahanNOAA, please see the latest changes to this PR, which I believe will resolve your concerns. The code is now working (ran the RT test; baselines change) and calculates the maximum random perturbation value based on namelist settings brought in from stochastic_physics.

physics/mp_thompson.meta

SamuelTrahanNOAA

Three dimension names are incorrect in the meta file. If you correct this, and update submodules, it compiles and runs on hera.intel. I also ran a -DDEBUG=ON test, and it ran as far as expected. (It failed in libpost due to enabling a post output variable it shouldn't.)

physics/mp_thompson.meta

JeffBeck-NOAA · 2022-04-07T21:30:26Z

Thanks, @SamuelTrahanNOAA. I missed this file in my latest commit. It's now fixed.

…ix/SPP

gthompsnWRF · 2022-04-08T14:31:20Z

physics/mp_thompson.F90

@@ -644,7 +650,9 @@ subroutine mp_thompson_run(ncol, nlev, con_g, con_rd,        &
                              diagflag=diagflag, do_radar_ref=do_radar_ref_mp,               &
                              has_reqc=has_reqc, has_reqi=has_reqi, has_reqs=has_reqs,       &
                              rand_perturb_on=spp_mp_opt, kme_stoch=kme_stoch,               &
-                              rand_pert=spp_wts_mp,                                          &
+                              rand_pert=spp_wts_mp, spp_var_list=spp_var_list,               &


It is sorta stupid request, but couldn't the new added arguments be listed in the same order in this block as in the block 40 lines lower? Just think it makes it easier. The order of arguments differs a little.

Sure, I can make that change.

gthompsnWRF · 2022-04-08T14:33:27Z

physics/mp_thompson.meta

+  dimensions = ()
+  type = integer
+  intent = in
+[spp_prt_list]


Clearly I am still being thick, but I don't see how the name "prt_list" gives me any clue of a magnitude of spp perturbation.

Yes, it could be better named. We followed the same naming convention for SPP as what was already in place for the land perturbation scheme in stochastic_physics. For that scheme, "lndp_prt_list" is used for the perturbation array. At some point, changing to something like *_mag_list would be more descriptive.

You could use a different name in mp_thompson than GFS_Typedefs so the variable will make sense in both contexts.

Since spp_prt_list is used across multiple physics suites and UFS submodules, I'd prefer keeping the name the same everywhere. We can certainly address this in a future PR, though.

…ix/SPP

Fix to original min_rand variable.

313d78f

JeffBeck-NOAA requested review from gthompsnWRF, RuiyuSun, climbfuji, SamuelTrahanNOAA, grantfirl, mzhangw, panll, mkavulich and ChunxiZhang-NOAA as code owners April 2, 2022 18:29

JeffBeck-NOAA changed the title ~~Fix to original min_rand variable.~~ Fix uninitialized min_rand variable in Thompson MP when using SPP Apr 2, 2022

JeffBeck-NOAA mentioned this pull request Apr 2, 2022

Fix uninitialized min_rand variable in Thompson MP when using SPP ufs-community/ufs-weather-model#1152

Merged

15 tasks

RuiyuSun approved these changes Apr 4, 2022

View reviewed changes

gthompsnWRF approved these changes Apr 4, 2022

View reviewed changes

grantfirl approved these changes Apr 4, 2022

View reviewed changes

ChunxiZhang-NOAA approved these changes Apr 4, 2022

View reviewed changes

SamuelTrahanNOAA requested changes Apr 4, 2022

View reviewed changes

ligiabernardet added the CCPP v6 Needed for CCPP v6 public release label Apr 5, 2022

JeffBeck-NOAA added 3 commits April 5, 2022 23:22

Pass SPP namelist entries into Thompson MP

6342e52

Add intent to SPP variables in meta file.

16993b9

Fix dimensions in the Thompson meta file

084551f

gthompsnWRF requested changes Apr 6, 2022

View reviewed changes

Fixes to application of rand_pert_max in Thompson MP.

a2fefa7

SamuelTrahanNOAA reviewed Apr 7, 2022

View reviewed changes

physics/mp_thompson.meta Outdated Show resolved Hide resolved

SamuelTrahanNOAA reviewed Apr 7, 2022

View reviewed changes

physics/mp_thompson.meta Outdated Show resolved Hide resolved

SamuelTrahanNOAA requested changes Apr 7, 2022

View reviewed changes

physics/mp_thompson.meta Outdated Show resolved Hide resolved

JeffBeck-NOAA added 2 commits April 7, 2022 21:28

Fix metadata descriptions.

044c900

Fix last metadata entry.

fdc9b2e

Merge branch 'main' of https://github.com/NCAR/ccpp-physics into bugf…

2e8f340

…ix/SPP

SamuelTrahanNOAA self-requested a review April 7, 2022 21:40

SamuelTrahanNOAA approved these changes Apr 7, 2022

View reviewed changes

SamuelTrahanNOAA requested a review from gthompsnWRF April 7, 2022 21:41

gthompsnWRF requested changes Apr 8, 2022

View reviewed changes

tanyasmirnova approved these changes Apr 8, 2022

View reviewed changes

JeffBeck-NOAA added 3 commits April 8, 2022 18:41

Reorder declarations.

0aab555

Merge branch 'main' of https://github.com/NCAR/ccpp-physics into bugf…

dc48d39

…ix/SPP

Merge branch 'main' of https://github.com/NCAR/ccpp-physics into bugf…

32b78e4

…ix/SPP

ChunxiZhang-NOAA approved these changes Apr 19, 2022

View reviewed changes

grantfirl merged commit e2806f0 into NCAR:main Apr 20, 2022

JeffBeck-NOAA mentioned this pull request Apr 20, 2022

Fix uninitialized min_rand variable in Thompson MP when using SPP NOAA-EMC/fv3atm#516

Merged

Fix uninitialized min_rand variable in Thompson MP when using SPP #892

Fix uninitialized min_rand variable in Thompson MP when using SPP #892

Conversation

JeffBeck-NOAA commented Apr 2, 2022 • edited Loading

Description

Issue(s) addressed

Testing

gthompsnWRF left a comment

Choose a reason for hiding this comment

SamuelTrahanNOAA left a comment

Choose a reason for hiding this comment

SamuelTrahanNOAA commented Apr 4, 2022

climbfuji commented Apr 4, 2022 via email

SamuelTrahanNOAA commented Apr 4, 2022

gthompsnWRF commented Apr 4, 2022

JeffBeck-NOAA commented Apr 4, 2022 • edited Loading

SamuelTrahanNOAA commented Apr 4, 2022

JeffBeck-NOAA commented Apr 4, 2022 • edited Loading

JeffBeck-NOAA commented Apr 4, 2022

SamuelTrahanNOAA commented Apr 4, 2022

gthompsnWRF commented Apr 4, 2022

climbfuji commented Apr 4, 2022 via email

JeffBeck-NOAA commented Apr 4, 2022 • edited Loading

SamuelTrahanNOAA commented Apr 4, 2022

SamuelTrahanNOAA commented Apr 4, 2022

gthompsnWRF commented Apr 5, 2022

JeffBeck-NOAA commented Apr 5, 2022

Choose a reason for hiding this comment

JeffBeck-NOAA Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBeck-NOAA Apr 6, 2022 • 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

JeffBeck-NOAA commented Apr 6, 2022

SamuelTrahanNOAA left a comment

Choose a reason for hiding this comment

JeffBeck-NOAA commented Apr 7, 2022

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

JeffBeck-NOAA Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBeck-NOAA commented Apr 2, 2022 •

edited

Loading

JeffBeck-NOAA commented Apr 4, 2022 •

edited

Loading

JeffBeck-NOAA commented Apr 4, 2022 •

edited

Loading

JeffBeck-NOAA commented Apr 4, 2022 •

edited

Loading

JeffBeck-NOAA Apr 6, 2022 •

edited

Loading

JeffBeck-NOAA Apr 6, 2022 •

edited

Loading

JeffBeck-NOAA Apr 8, 2022 •

edited

Loading