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.

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

adding updated RAS #585

Merged
merged 5 commits into from
Mar 17, 2021
Merged

Conversation

SMoorthi-emc
Copy link
Collaborator

@SMoorthi-emc SMoorthi-emc commented Mar 4, 2021

This PR is to replace current RAS with an updated one that fixes a potential issue that can cause model crash.

Further changes:

  • In MG2 and MG3 the single if loop belowif (lamr(i,k) > qsmall .and. one/lamr(i,k) < Dcs) then is expanded to two nested ifs like
if (lamr(i,k) > qsmall) then
if (one/lamr(i,k) < Dcs) then

because on some computers the first if may lead to divide by zero leading to crash.

  • In radsw_main.F90, the code is slightly rewritten to avoid sqrt of a negative number and potential divide by zero.
    These issues were encountered during debugging. After fixing these two issues runs haven't crashed.
  • reactivate 3d mass flux diagnostic tendencies in GFS_DCNV_generic_post (from @climbfuji).

Associated PRs:

#585
NOAA-EMC/fv3atm#252
ufs-community/ufs-weather-model#448

For regression testing, see ufs-community/ufs-weather-model#448

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

Looks good to me. This PR will be combined with an fv3atm PR from my side (for updating the submodule pointer for ccpp-physics and potential updates in GFS_typedefs.F90, if needed), and a ufs-weather-model PR that enables the fv3_rasmgshoc regression test.

@@ -8,7 +8,7 @@ module rascnv
implicit none
public :: rascnv_init, rascnv_run, rascnv_finalize
private
logical :: is_initialized = .False.
logical, save :: is_initialized = .False.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be necessary, module variables are by default 'save' variables. But it doesn't hurt to have it. Same for all of the module variables below.


CNV_MFD(ipt,ll) = CNV_MFD(ipt,ll) + flx(ib)/dt

! CNV_DQLDT(ipt,ll) = CNV_DQLDT(ipt,ll)
! & + max(0.,(QLI(ib)+QII(ib)-qiid-qlid))/dt
!! CNV_DQLDT(ipt,ll) = CNV_DQLDT(ipt,ll)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these double exclamation marks !! intentional? They will probably be picked by doxygen when creating the scientific documentation (same a few lines below).

@SMoorthi-emc
Copy link
Collaborator Author

SMoorthi-emc commented Mar 4, 2021 via email

@climbfuji
Copy link
Collaborator

Dom, That double "!!" was intentional to the extent that it is different from the single "!". No particular significance. Moorthni

Ok, thanks. @ligiabernardet as our doxygen expert, do you think these !! will be picked up mistakenly by doxygen and end up in the scientific documentation?

@ligiabernardet
Copy link
Collaborator

Dom, That double "!!" was intentional to the extent that it is different from the single "!". No particular significance. Moorthni

Ok, thanks. @ligiabernardet as our doxygen expert, do you think these !! will be picked up mistakenly by doxygen and end up in the scientific documentation?

@mzhangw Can you please check this?

@mzhangw
Copy link
Collaborator

mzhangw commented Mar 5, 2021

It is OK with single line starting with "!!"

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

@SMoorthi-emc can you also say something about the MG microphysics changes and the radsw_main.F90 changes, please? The description of the PR says only two files are changed (RAS .F90 and .meta), plus what I added for the DCNV convective mass flux tendencies. Thank you!

@climbfuji
Copy link
Collaborator

@mjiacono there are some small changes in radsw_main.F90, do you want to take a look?

@climbfuji climbfuji requested a review from junwang-noaa March 15, 2021 22:57
@SMoorthi-emc
Copy link
Collaborator Author

In MG2 and MG3 the single if loop below
"if (lamr(i,k) > qsmall .and. one/lamr(i,k) < Dcs) then" is expanded to two nested ifs like
"if (lamr(i,k) > qsmall) then
if (one/lamr(i,k) < Dcs) then".
this is done based on Dom's recommendation as on some computers the first if may lead to divide by zero leading to crash.
In the radsw_main.F90, the code is slightly rewritten to avoid sqrt of a negative number and potential divide by zero.
These issues were encountered during debugging. After fixing these two issues runs haven't crashed.

@climbfuji
Copy link
Collaborator

In MG2 and MG3 the single if loop below
"if (lamr(i,k) > qsmall .and. one/lamr(i,k) < Dcs) then" is expanded to two nested ifs like
"if (lamr(i,k) > qsmall) then
if (one/lamr(i,k) < Dcs) then".
this is done based on Dom's recommendation as on some computers the first if may lead to divide by zero leading to crash.
In the radsw_main.F90, the code is slightly rewritten to avoid sqrt of a negative number and potential divide by zero.
These issues were encountered during debugging. After fixing these two issues runs haven't crashed.

Great, thank you Moorthi. I'll copy & paste this into the description at the top.

@mjiacono
Copy link
Collaborator

mjiacono commented Mar 16, 2021 via email

@climbfuji
Copy link
Collaborator

Dom, The proposed changes to radsw_main.F90 look OK to me. Mike From: Dom Heinzeller @.> Reply-To: NCAR/ccpp-physics @.> Date: Monday, March 15, 2021 at 6:51 PM To: NCAR/ccpp-physics @.> Cc: "Iacono, Mike" @.>, Mention @.***> Subject: Re: [NCAR/ccpp-physics] adding updated RAS (#585) Caution: This email originated outside of the organization @mjiacono<https://urldefense.com/v3/https:/github.com/mjiacono;!!CAFLEWIB!WrNJiyZ8s3SKcPOIqf4mYADNJ0j_sQXHbyyyPunR36-PU8zw6tQaOWR90j4$> there are some small changes in radsw_main.F90, do you want to take a look? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://urldefense.com/v3/https:/github.com/NCAR/ccpp-physics/pull/585*issuecomment-799807013;Iw!!CAFLEWIB!WrNJiyZ8s3SKcPOIqf4mYADNJ0j_sQXHbyyyPunR36-PU8zw6tQadTCVbWo$>, or unsubscribe<https://urldefense.com/v3/https:/github.com/notifications/unsubscribe-auth/AE7W55D3WMIH35QAA6V4HTTTD2FPJANCNFSM4YSIKIKQ;!!CAFLEWIB!WrNJiyZ8s3SKcPOIqf4mYADNJ0j_sQXHbyyyPunR36-PU8zw6tQaEaOv7Js$>.

Thanks, Mike!

@climbfuji climbfuji merged commit 7242a6d into NCAR:master Mar 17, 2021
@SMoorthi-emc
Copy link
Collaborator Author

SMoorthi-emc commented Mar 17, 2021 via email

@grantfirl grantfirl mentioned this pull request Apr 15, 2021
HelinWei-NOAA pushed a commit to HelinWei-NOAA/ccpp-physics that referenced this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants