Skip to content
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

Jump to bottom

Update to MYNN surface layer scheme #131

Merged
merged 3 commits into from
May 19, 2022
Merged

Update to MYNN surface layer scheme #131

merged 3 commits into from
May 19, 2022

Conversation

joeolson42
Copy link
Collaborator

@joeolson42 joeolson42 commented Mar 25, 2022 •
edited
Loading

Description

  1. Bug fix for COARE3.5 - this option is not used by default so it will not cause a change in results.
  2. Bug fix for computing theta and rho at k=1 (was using p_sfc instead of p(k=1)).
  3. Removed time averaging of u* over the water to be more similar to surface heat flux calculation in ocean_sfc.
  4. Moved 4 internal parameters to namelist options:
  • isftcflx (default = 0)
  • iz0tlnd (default = 0)
  • sfclay_compute_flux (default = .false. which will change results of regression tests)
  • sfclay_compute_diag (default = .false.)

Edit again by @SamuelTrahanNOAA The regional_spp_sppt_shum_skeb fails in this PR, but that is not due to this PR's changes. The thompson scheme uses uninitialized memory when stochastic physics is enabled, and that's what regional_spp_sppt_shum_skeb tests. This PR changes totally unrelated portions of the code, but it is just enough to put nonphysical values in the uninitialized thompson scheme variables. PR ufs-community#1152 in the community repository should fix this.

"To Do" List

@SamuelTrahanNOAA
Copy link
Collaborator

This change requires modifications to two regression tests, which @SamuelTrahanNOAA is working on now.

@tanyasmirnova
Copy link
Collaborator

@joeolson42 @SamuelTrahanNOAA I think it would be good to have the names of these new namelist options related to the MYNN surface layer scheme, for example: change sfclay_compute_flux to mynnsfc_compute_flux in GFS_typedefs.F90/meta.

tanyasmirnova
tanyasmirnova reviewed Mar 31, 2022
View reviewed changes
Copy link
Collaborator

@tanyasmirnova tanyasmirnova left a comment

Choose a reason for hiding this comment

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

@SamuelTrahanNOAA Sam, is tests/tests/hrrr_control should be changed?

@SamuelTrahanNOAA
Copy link
Collaborator

Sam, is tests/tests/hrrr_control should be changed

I don't know. I didn't make that test.

@joeolson42
Copy link
Collaborator Author

joeolson42 commented Mar 31, 2022 via email

@SamuelTrahanNOAA
Copy link
Collaborator

@joeolson42 @tanyasmirnova I think you're both telling me to remove the nst_anl and nstf_name options from the namelist. (That'll make nst_anl false rather than true.) I'll run that now.

@SamuelTrahanNOAA
Copy link
Collaborator

I think you're both telling me to remove the nst_anl and nstf_name options from the namelist. (That'll make nst_anl false rather than true.) I'll run that now.

Results are the same, so I've removed those two lines.

@SamuelTrahanNOAA
Copy link
Collaborator

One of the tests segfaulted. Joe is debugging, but if anyone else can look at it, that may speed us up. I've copied the failed test here:

HERA: /scratch2/BMC/wrfruc/Samuel.Trahan/gsl-PR/PR131/SEGFAULT

@tanyasmirnova
Copy link
Collaborator

@SamuelTrahanNOAA @joeolson42 It looks like this test runs FV3_HRRR but has in input.nml:
nst_anl = .true.
nstf_name = 2, 1, 0, 0, 0
These line should not be there.

@SamuelTrahanNOAA
Copy link
Collaborator

These line should not be there.

I've removed them, and I'm rerunning the test.

@SamuelTrahanNOAA
Copy link
Collaborator

It still segfaults:

/scratch2/BMC/wrfruc/Samuel.Trahan/gsl-PR/PR131/AGAIN_SEGFAULT

@tanyasmirnova
Copy link
Collaborator

@joeolson42 @SamuelTrahanNOAA It crashes in MYNN surface: 7: libpthread-2.17.s 00002AAFE5512630 Unknown Unknown Unknown
7: fv3.exe 0000000003369872 module_sf_mynn_mp 3756 module_sf_mynn.F90
7: fv3.exe 0000000003369469 module_sf_mynn_mp 3494 module_sf_mynn.F90

@joeolson42
Copy link
Collaborator Author

joeolson42 commented Mar 31, 2022 via email

@SamuelTrahanNOAA
Copy link
Collaborator

I recompiled in debug mode, and it gives a more useful response:

106: forrtl: error (182): floating invalid - possible uninitialized real/complex variable.
106: Image              PC                Routine            Line        Source
106: fv3.exe            000000000AE8A8EE  Unknown               Unknown  Unknown
106: libpthread-2.17.s  00002AF0D14E2630  Unknown               Unknown  Unknown
106: fv3.exe            0000000009416FD7  module_mp_thompso        1295  module_mp_thompson.F90
106: fv3.exe            0000000009754A84  mp_thompson_mp_mp         635  mp_thompson.F90
106: fv3.exe            00000000071CA248  ccpp_fv3_hrrr_phy       12648  ccpp_FV3_HRRR_physics_cap.F90
106: fv3.exe            0000000006B44C37  ccpp_static_api_m         662  ccpp_static_api.F90
106: fv3.exe            0000000006B37E60  ccpp_driver_mp_cc         188  CCPP_driver.F90

@SamuelTrahanNOAA
Copy link
Collaborator

The community repository also fails that test if -DDEBUG=ON is enabled. It seems the test was already broken in the same way, but Joe's unrelated changes pushed it just over the edge. This is not surprising when there's an uninitialized variable:

 35: forrtl: error (182): floating invalid - possible uninitialized real/complex variable.
 35: Image              PC                Routine            Line        Source             
 35: fv3.exe            000000000AE8A8EE  Unknown               Unknown  Unknown
 35: libpthread-2.17.s  00002B05702DE630  Unknown               Unknown  Unknown
 35: fv3.exe            0000000009416FD7  module_mp_thompso        1295  module_mp_thompson.F90
 35: fv3.exe            0000000009754A84  mp_thompson_mp_mp         635  mp_thompson.F90
 35: fv3.exe            00000000071CA248  ccpp_fv3_hrrr_phy       12648  ccpp_FV3_HRRR_physics_cap.F90
 35: fv3.exe            0000000006B44C37  ccpp_static_api_m         662  ccpp_static_api.F90
 35: fv3.exe            0000000006B37E60  ccpp_driver_mp_cc         188  CCPP_driver.F90
 35: libiomp5.so        00002B056E9EEA43  __kmp_invoke_micr     Unknown  Unknown
 35: libiomp5.so        00002B056E9B1CDA  Unknown               Unknown  Unknown
 35: libiomp5.so        00002B056E9B35B6  __kmp_fork_call       Unknown  Unknown

The error is here:

         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
Copy link
Collaborator

The stochastic physics runs at the end of the physics timestep, which is after microphysics. The rand_pert is probably uninitialized in the first timestep.

@SamuelTrahanNOAA
Copy link
Collaborator

Also, nothing is ever assigned to min_rand. I don't know what the author was intending, but it won't work until min_rand=something

@joeolson42
Copy link
Collaborator Author

joeolson42 commented Apr 1, 2022 via email

@SamuelTrahanNOAA
Copy link
Collaborator

I did some code clean-up in the surface layer scheme. I'd like to commit
that. Nothing else (since there was no debugging success to fix a problem).

Where? Is there a PR for this?

@joeolson42
Copy link
Collaborator Author

joeolson42 commented Apr 1, 2022 via email

@SamuelTrahanNOAA
Copy link
Collaborator

You need to remove the current changes first, as they are breaking the other regression test.

@joeolson42
Copy link
Collaborator Author

joeolson42 commented Apr 1, 2022 via email

@SamuelTrahanNOAA
Copy link
Collaborator

SamuelTrahanNOAA commented Apr 1, 2022 •
edited
Loading

The test crashes due to an invalid floating point operation. The community would not find that acceptable, so if this is PR'd to the community repos, it would be rejected.

Edit: The error is "floating invalid - possible uninitialized real/complex variable" not a segmentation violation.

@SamuelTrahanNOAA
Copy link
Collaborator

I submitted an issue here:

NCAR/ccpp-physics#891

@joeolson42
Copy link
Collaborator Author

joeolson42 commented Apr 1, 2022 via email

@SamuelTrahanNOAA
Copy link
Collaborator

I also submitted this PR, to remove the broken regression test:

ufs-community#1151

@SamuelTrahanNOAA
Copy link
Collaborator

I want to be clear: the failing regression test is not due to changes in this PR. It is due to that regression test's use of uninitialized memory. It is unreasonable to expect that test to work at all, and its failures should not hold up valid development.

@SamuelTrahanNOAA
Copy link
Collaborator

I meant after the stochastic physics bug is fixed, then we should be back to these simple changes to the mynn sfc layer code

Yes, but there's no telling how long it would take to fix the stochastic physics. I would rather remove that regression test until its bugs are fixed.

@SamuelTrahanNOAA SamuelTrahanNOAA mentioned this pull request Apr 27, 2022
Merged
16 tasks
@SamuelTrahanNOAA
Copy link
Collaborator

These changes are in ufs-community#1195 and will be merged back to gsl/develop after the community repositories are updated.

@SamuelTrahanNOAA
Copy link
Collaborator

#138 will merge these changes

@SamuelTrahanNOAA SamuelTrahanNOAA mentioned this pull request May 18, 2022
Merged
16 tasks
@SamuelTrahanNOAA SamuelTrahanNOAA merged commit cedbccc into NOAA-GSL:gsl/develop May 19, 2022
@joeolson42
Copy link
Collaborator Author

joeolson42 commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SamuelTrahanNOAA SamuelTrahanNOAA SamuelTrahanNOAA approved these changes

@tanyasmirnova tanyasmirnova tanyasmirnova approved these changes

@mdtoyNOAA mdtoyNOAA mdtoyNOAA approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joeolson42 @SamuelTrahanNOAA @tanyasmirnova @mdtoyNOAA