Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
grantfirl merged 11 commits into from
Apr 20, 2022
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
313d78f
Fix to original min_rand variable.
JeffBeck-NOAA Apr 2, 2022
6342e52
Pass SPP namelist entries into Thompson MP
JeffBeck-NOAA Apr 5, 2022
16993b9
Add intent to SPP variables in meta file.
JeffBeck-NOAA Apr 6, 2022
084551f
Fix dimensions in the Thompson meta file
JeffBeck-NOAA Apr 6, 2022
a2fefa7
Fixes to application of rand_pert_max in Thompson MP.
JeffBeck-NOAA Apr 6, 2022
044c900
Fix metadata descriptions.
JeffBeck-NOAA Apr 7, 2022
fdc9b2e
Fix last metadata entry.
JeffBeck-NOAA Apr 7, 2022
2e8f340
Merge branch 'main' of https://github.com/NCAR/ccpp-physics into bugf…
JeffBeck-NOAA Apr 7, 2022
0aab555
Reorder declarations.
JeffBeck-NOAA Apr 8, 2022
dc48d39
Merge branch 'main' of https://github.com/NCAR/ccpp-physics into bugf…
JeffBeck-NOAA Apr 8, 2022
32b78e4
Merge branch 'main' of https://github.com/NCAR/ccpp-physics into bugf…
JeffBeck-NOAA Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions physics/module_mp_thompson.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -984,7 +984,8 @@ SUBROUTINE mp_gt_driver(qv, qc, qr, qi, qs, qg, ni, nr, nc, &
has_reqc, has_reqi, has_reqs, &
rand_perturb_on, &
kme_stoch, &
rand_pert, &
rand_pert, spp_prt_list,spp_var_list &
spp_stddev_cutoff,n_var_spp &
ids,ide, jds,jde, kds,kde, & ! domain dims
ims,ime, jms,jme, kms,kme, & ! memory dims
its,ite, jts,jte, kts,kte, & ! tile dims
Expand Down Expand Up @@ -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, &

@gthompsnWRF gthompsnWRF Apr 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@JeffBeck-NOAA JeffBeck-NOAA Apr 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

spp_var_list

INTEGER, INTENT(IN):: has_reqc, has_reqi, has_reqs
#if ( WRF_CHEM == 1 )
Expand Down Expand Up @@ -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

@gthompsnWRF gthompsnWRF Apr 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@JeffBeck-NOAA JeffBeck-NOAA Apr 6, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

INTEGER:: i, j, k, m
INTEGER:: imax_qc,imax_qr,imax_qi,imax_qs,imax_qg,imax_ni,imax_nr
INTEGER:: jmax_qc,jmax_qr,jmax_qi,jmax_qs,jmax_qg,jmax_ni,jmax_nr
Expand Down Expand Up @@ -1268,6 +1270,19 @@ SUBROUTINE mp_gt_driver(qv, qc, qr, qi, qs, qg, ni, nr, nc, &
kmax_ni = 0
kmax_nr = 0

!Get the Thompson MP SPP magnitude and standard deviation cutoff

if (rand_perturb_on .ne. 0) then
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)

@gthompsnWRF gthompsnWRF Apr 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

@JeffBeck-NOAA JeffBeck-NOAA Apr 6, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

end select
enddo
endif

print*, ' spp_mp_mag_times_cutoff is = ', spp_mp_mag_times_cutoff

@gthompsnWRF gthompsnWRF Apr 6, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@JeffBeck-NOAA JeffBeck-NOAA Apr 6, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


j_loop: do j = j_start, j_end
i_loop: do i = i_start, i_end

Expand All @@ -1292,7 +1307,7 @@ SUBROUTINE mp_gt_driver(qv, qc, qr, qi, qs, qg, ni, nr, nc, &
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))
if (MOD(m,2) .ne. 0) rand3 = 0.25*(rand_pert(i,1)+spp_mp_mag_times_cutoff)
m = RSHIFT(ABS(rand_perturb_on),3)
endif
!+---+-----------------------------------------------------------------+
Expand Down
12 changes: 10 additions & 2 deletions physics/mp_thompson.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,11 @@ subroutine mp_thompson_run(ncol, nlev, con_g, con_rd, &

! SPP
integer, intent(in) :: spp_mp
integer, intent(in) :: n_var_spp
real(kind_phys), intent(in) :: spp_wts_mp(:,:)
real(kind_phys), intent(in) :: spp_prt_list(:)
character(len=3), intent(in) :: spp_var_list(:)
real(kind_phys), intent(in) :: spp_stddev_cutoff(:)

! Local variables

Expand Down Expand Up @@ -644,7 +648,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_out, &
spp_prt_list=spp_prt_list_out,n_var_spp=n_var_spp, &
spp_stddev_cutoff=spp_stddev_cutoff_out, &
ids=ids, ide=ide, jds=jds, jde=jde, kds=kds, kde=kde, &
ims=ims, ime=ime, jms=jms, jme=jme, kms=kms, kme=kme, &
its=its, ite=ite, jts=jts, jte=jte, kts=kts, kte=kte, &
Expand Down Expand Up @@ -681,7 +687,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_prt_list=spp_prt_list_out, &
spp_stddev_cutoff=spp_stddev_cutoff_out,n_var_spp=n_var_spp, &
spp_var_list=spp_var_list_out, &
ids=ids, ide=ide, jds=jds, jde=jde, kds=kds, kde=kde, &
ims=ims, ime=ime, jms=jms, jme=jme, kms=kms, kme=kme, &
its=its, ite=ite, jts=jts, jte=jte, kts=kts, kte=kte, &
Expand Down
23 changes: 23 additions & 0 deletions physics/mp_thompson.meta
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,29 @@
dimensions = ()
type = integer
intent = in
[n_var_spp]
standard_name = number_of_perturbed_spp_schemes
long_name = number of perturbed spp schemes
units = count
dimensions = ()
type = integer
intent = in
[spp_prt_list]

@gthompsnWRF gthompsnWRF Apr 8, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@JeffBeck-NOAA JeffBeck-NOAA Apr 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@SamuelTrahanNOAA SamuelTrahanNOAA Apr 8, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@JeffBeck-NOAA JeffBeck-NOAA Apr 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

standard_name = magnitude_of_spp_perturbations
long_name = magnitude of spp perturbations
units = 1
dimensions = ()
type = real
kind = kind_phys
intent = in
[spp_stddev_cutoff]
standard_name = magnitude_of_spp_standard_deviation_cutoff
long_name = magnitude of spp standard deviation cutoff
units = 1
dimensions = ()
type = real
kind = kind_phys
intent = in
[errmsg]
standard_name = ccpp_error_message
long_name = error message for error handling in CCPP
Expand Down