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

Minor modification and bug fix in GFS cumulus convection schemes #232

Open
wants to merge 22 commits into
base: ufs/dev
Choose a base branch
from

Conversation

rhaesung
Copy link
Collaborator

@rhaesung rhaesung commented Oct 31, 2024

The code of this PR was provided by @JongilHan66.

  1. Modified prognostic updraft fraction (sigmab) calculation in 'progsigma_calc.f90' which is physically more sound:
    a) moisture convergence calculation: integrate from the convection source level rather than from the cloud base
    b) 2D advection of sigmab: sigmab advection averaged over the cloud layers rather than taking a maximum sigmab advection from k=2 to the model top
    c) To suppress unrealistically large reflectivity in the model first time step, minimum sigmab at the first time step is set to zero

  2. Fix in missing vertical transport of turbulent kinetic energy (TKE) when aerosol transport is turned on (samfdeepcnv.f & samfshalcnv.f)

  3. Introduce TKE at model layer interfaces (tkeh) for use in convection schemes (CCPP_typedefs.F90, CCPP_typedefs.meta, satmedmfvdifq.F, satmedmfvdifq.meta, samfdeepcnv.f, samfdeepcnv.meta, samfshalcnv.f, and samfshalcnv.meta)

  4. Vertical transports of hydrometeor variables are currently not allowed in the convection schemes. But vertical transports of number concentrations of only cloud water and ice are mistakenly allowed, which is fixed in this update (CCPP_typedefs.F90)

  5. All the modifications and bug fixes above had neutral impacts on the GFS forecasts

@JongilHan66
Copy link
Collaborator

@rhaesung Please add one more under item 1. in the PR description:
c) To suppress unrealistically large reflectivity in the model first time step, minimum sigmab at the first time step is set to zero

@rhaesung
Copy link
Collaborator Author

@rhaesung Please add one more under item 1. in the PR description: c) To suppress unrealistically large reflectivity in the model first time step, minimum sigmab at the first time step is set to zero

@JongilHan66 Done!

Copy link
Collaborator

@lisa-bengtsson lisa-bengtsson 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!

Copy link
Collaborator

@dustinswales dustinswales left a comment

Choose a reason for hiding this comment

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

@rhaesung These changes all look good to me.
I have a few tiny suggestions for the precision to make the code more portable.

Copy link
Collaborator

@grantfirl grantfirl 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, @rhaesung

@grantfirl grantfirl requested a review from climbfuji November 15, 2024 16:05
Copy link

@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 ok to me.

@yangfanglin
Copy link
Collaborator

What is the schedule for merging this PR ?

@grantfirl
Copy link
Collaborator

What is the schedule for merging this PR ?

@yangfanglin I think that the plan was to try to merge the remaining PRs from the RRFS branch and then do this one. If this is a higher priority than the RRFS ones, we can switch up. FYI @jkbk2004

@rhaesung
Copy link
Collaborator Author

@grantfirl Thanks for catching this! I'll coordinate with @JongilHan66 to update the scheme based on your feedback.

@JongilHan66
Copy link
Collaborator

@lisa-bengtsson Yes. Initial value of sigmab=0.03 tended to produce unrealistically large reflectivity at the initial time especially in the RRFS.

@lisa-bengtsson
Copy link
Collaborator

@lisa-bengtsson Yes. Initial value of sigmab=0.03 tended to produce unrealistically large reflectivity at the initial time especially in the RRFS.

@JongilHan66 I recall now... it's taken some time for this PR to get through the queue.

@rhaesung
Copy link
Collaborator Author

rhaesung commented Feb 13, 2025

@JongilHan66 @grantfirl I confirmed that the changes in progsigma_calc.f90 in PR #252 prevent the completion of the cpld_restart_gfsv17 test when compared to the new baseline, with the following error:

120: FATAL from PE 120: NaN in input field of mpp_reproducing_sum(_2d), this indicates numerical instability

@JongilHan66 Could you please review the logic for sigmind_new to ensure it doesn’t result in invalid values during the simulation? I think we need something like the if(flag_init .and. .not. flag_restart) condition in line 66, since the restart run is failing?

@JongilHan66
Copy link
Collaborator

@rhaesung I don't think sigmind_new causes the instability as it is a local constant variable (minimum updraft fraction; sigmind=0.01). Have you run it with the original progsigma_calc.f90 before the modification?

@rhaesung
Copy link
Collaborator Author

@rhaesung I don't think sigmind_new causes the instability as it is a local constant variable (minimum updraft fraction; sigmind=0.01). Have you run it with the original progsigma_calc.f90 before the modification?

@JongilHan66 Yes, I recall that the develop branch worked fine. Please review the change in Jili’s PR 252. Since this change prevents the cpld_restart_gfsv17 test from completing with the above error when compared to the new baseline, I suspect the issue is related to the proper handling of sigmind_new and sigmind during the simulation.

@JongilHan66
Copy link
Collaborator

@rhaesung The changes associated with sigmind_new & sigmind follow Jili's changes. And I find the current changes are exactly the same as Jili's PR252. In the restart, sigmind_new will be sigmind=0.01. I don't get it why the constant sigind_new causes a problem in restart. How about setting sigmind_new=0.0 before line 66 (i.e., if (flag_init) then)?

@JongilHan66
Copy link
Collaborator

@rhaesung @lisa-bengtsson I thought that if flag_restart=.true., flag_init automatically becomes .false, so sigmind_new=sigmind=0.01. Am I wrong?

@lisa-bengtsson
Copy link
Collaborator

@rhaesung @lisa-bengtsson I thought that if flag_restart=.true., flag_init automatically becomes .false, so sigmind_new=sigmind=0.01. Am I wrong?

I thought so too, but it turned out flag_restart can be true and the flag_init can be true at the same time - indicating that it is the first time-step under restart. That is why I put the .not. flag_restart condition in there.

@JongilHan66
Copy link
Collaborator

@lisa-bengtsson Thanks for the clarification!! @rhaesung Please change the 66 line to if(flag_init .and. .not. flag_restart). But I don't think this change can avoid the numerical instability in restart.

@rhaesung
Copy link
Collaborator Author

@JongilHan66 @lisa-bengtsson @grantfirl Okay, I confirmed that if(flag_init .and. .not. flag_restart) is at least resolving the issue in PR 252. I’ll re-test the failed tests with this change for this PR.

@rhaesung
Copy link
Collaborator Author

@JongilHan66 I was able to get all the failed tests to pass. Please re-review the changes here and at NOAA-EMC/fv3atm#885, and let me know if you have any concerns.

@grantfirl
Copy link
Collaborator

@rhaesung Great work debugging. Can you please merge ufs/dev into this branch and fix the conflict? progsigma_calc.f90 was modified in the last merged PR, so I'm guessing that is where the conflict snuck in.

@rhaesung
Copy link
Collaborator Author

@rhaesung Great work debugging. Can you please merge ufs/dev into this branch and fix the conflict? progsigma_calc.f90 was modified in the last merged PR, so I'm guessing that is where the conflict snuck in.

@grantfirl Done! I've merged ufs/dev into this branch and resolved the conflict.

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.

9 participants