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 #2487

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from

Conversation

rhaesung
Copy link
Contributor

@rhaesung rhaesung commented Oct 31, 2024

Commit Queue Requirements:

  • Fill out all sections of this template.
  • All sub component pull requests have been reviewed by their code managers.
  • Run the full Intel+GNU RT suite (compared to current baselines) on either Hera/Derecho/Hercules
  • Commit 'test_changes.list' from previous step

Description:

This PR includes minor modifications and bug fixes to the GFS cumulus convection schemes, enhancing their performance and addressing identified issues. 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 (GFS_typedefs.F90, GFS_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

Commit Message:

* UFSWM - 
  * FV3 - 
    * ccpp-physics - 

Priority:

  • Normal

Git Tracking

UFSWM:

  • None

Sub component Pull Requests:



Testing Log:

  • RDHPCS
    • Hera
    • Orion
    • Hercules
    • Jet
    • Gaea
    • Derecho
  • WCOSS2
    • Dogwood/Cactus
    • Acorn
  • CI
  • opnReqTest (complete task if unnecessary)

@jkbk2004
Copy link
Collaborator

@rhaesung Can I check your holiday PTO schedule this week? If you are available tomorrow, we can schedule to work on this pr.

@rhaesung
Copy link
Contributor Author

@rhaesung Can I check your holiday PTO schedule this week? If you are available tomorrow, we can schedule to work on this pr.

@jkbk2004 I will work tomorrow.

@jkbk2004
Copy link
Collaborator

@FernandoAndrade-NOAA @BrianCurtis-NOAA #2483 has ACS subcomponent PR as well. I think people will be away for holiday. Instead, can we schedule to work on this pr? @rhaesung can you sync up branches?

@BrianCurtis-NOAA
Copy link
Collaborator

FV3 needs their CM's to approve it.

@BrianCurtis-NOAA
Copy link
Collaborator

There's WW3 changes but no WW3 PR linked.

@jkbk2004
Copy link
Collaborator

Sync issue at WW3. @rhaesung please, go ahead to sync up branches across sub components and weather model level.

@jkbk2004
Copy link
Collaborator

@FernandoAndrade-NOAA @BrianCurtis-NOAA FYI: If @grantfirl @dustinswales are off this week, we may not be able to do merging this week.

@DeniseWorthen
Copy link
Collaborator

DeniseWorthen commented Nov 25, 2024

The test_changes.list hasn't been merged properly.

@rhaesung
Copy link
Contributor Author

@jkbk2004 Okay, it should be good to go. Please let me know if you encounter any issues.

@BrianCurtis-NOAA
Copy link
Collaborator

the test_changes.list wasn't updated properly, lots of merge conflicts there. Unless you have the original test_changes.list from when you ran your initial full suite test, it might be easiest to ask @jkbk2004 to run the full suite again to generate an updated test_changes.list

@rhaesung
Copy link
Contributor Author

the test_changes.list wasn't updated properly, lots of merge conflicts there. Unless you have the original test_changes.list from when you ran your initial full suite test, it might be easiest to ask @jkbk2004 to run the full suite again to generate an updated test_changes.list

Sorry for the confusion—I'm still getting familiar with the process, and I don't have the original test_changes.list file. @jkbk2004, could you please help by running the full suite to generate a fresh test_changes.list?

@DeniseWorthen
Copy link
Collaborator

@rhaesung When you merge develop and your test_changes.list is different (so you have conflicts), what I do is

 git checkout --ours tests/test_changes.list

@rhaesung
Copy link
Contributor Author

@DeniseWorthen Great, thanks for the tip! I appreciate the guidance.

@grantfirl
Copy link
Collaborator

@rhaesung Could you please pull in the latest develop to this branch (same for fv3atm and ufs/dev for ccpp-physics)?

@jkbk2004
Copy link
Collaborator

@rhaesung
Copy link
Contributor Author

@jkbk2004 Done. Please let me know if you see any other issues.

@jkbk2004 jkbk2004 added Baseline Updates Current baselines will be updated. Ready for Commit Queue The PR is ready for the Commit Queue. All checkboxes in PR template have been checked. labels Jan 28, 2025
@jkbk2004
Copy link
Collaborator

@BrianCurtis-NOAA as there is still some discussion on #2529: cubed sphere, we are about to start working on this pr.

@jkbk2004 jkbk2004 removed the Ready for Commit Queue The PR is ready for the Commit Queue. All checkboxes in PR template have been checked. label Jan 29, 2025
@jkbk2004
Copy link
Collaborator

jkbk2004 commented Jan 29, 2025

@rhaesung quite some cases are failing when comparing with new baseline. It happens across machines. no crash and baseline creation itself is ok. I suspect some random behavior is happening. There is no problem with develop branch. Sounds like some debugging is needed. @BrianCurtis-NOAA @FernandoAndrade-NOAA FYI: Failed baseline comparison cases are

cpld_restart_gfsv17 intel
cpld_2threads_p8 intel
cpld_control_c192_p8 intel
cpld_restart_c192_p8 intel
cpld_bmark_p8 intel
cpld_restart_bmark_p8 intel
control_c384 intel
control_c384gdas intel
control_2threads_p8 intel
hafs_regional_atm intel
hafs_regional_atm_thompson_gfdlsf intel
hafs_regional_1nest_atm intel
hafs_regional_telescopic_2nests_atm intel
hafs_global_1nest_atm intel
hafs_global_multiple_4nests_atm intel
hafs_regional_specified_moving_1nest_atm intel
hafs_regional_storm_following_1nest_atm intel
hafs_regional_storm_following_1nest_atm_ocn intel
hafs_global_storm_following_1nest_atm intel
hafs_regional_storm_following_1nest_atm_ocn_debug intel
hafs_regional_storm_following_1nest_atm_ocn_wav intel
hafs_regional_storm_following_1nest_atm_ocn_wav_inline intel
hafs_regional_storm_following_1nest_atm_ocn_wav_mom6 intel
hafs_regional_storm_following_1nest_atm_ocn intelllvm
hafs_regional_storm_following_1nest_atm_ocn_debug intelllvm

@RuiyuSun
Copy link
Contributor

@rhaesung quite some cases are failing when comparing with new baseline. It happens across machines. no crash and baseline creation itself is ok. I suspect some random behavior is happening. There is no problem with develop branch. Sounds like some debugging is needed. @BrianCurtis-NOAA @FernandoAndrade-NOAA FYI: Failed baseline comparison cases are

cpld_restart_gfsv17 intel
cpld_2threads_p8 intel
cpld_control_c192_p8 intel
cpld_restart_c192_p8 intel
cpld_bmark_p8 intel
cpld_restart_bmark_p8 intel
control_c384 intel
control_c384gdas intel
control_2threads_p8 intel
hafs_regional_atm intel
hafs_regional_atm_thompson_gfdlsf intel
hafs_regional_1nest_atm intel
hafs_regional_telescopic_2nests_atm intel
hafs_global_1nest_atm intel
hafs_global_multiple_4nests_atm intel
hafs_regional_specified_moving_1nest_atm intel
hafs_regional_storm_following_1nest_atm intel
hafs_regional_storm_following_1nest_atm_ocn intel
hafs_global_storm_following_1nest_atm intel
hafs_regional_storm_following_1nest_atm_ocn_debug intel
hafs_regional_storm_following_1nest_atm_ocn_wav intel
hafs_regional_storm_following_1nest_atm_ocn_wav_inline intel
hafs_regional_storm_following_1nest_atm_ocn_wav_mom6 intel
hafs_regional_storm_following_1nest_atm_ocn intelllvm
hafs_regional_storm_following_1nest_atm_ocn_debug intell

This PR is supposed to change the results.

@jkbk2004
Copy link
Collaborator

yes, baseline changes are expected. New baseline creation is ok but when it compares with new baseline, it fails.

@RuiyuSun
Copy link
Contributor

Are the results not reproducible with this change?

@RuiyuSun
Copy link
Contributor

@JongilHan66 Could you take a look at this ?

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Feb 5, 2025

@rhaesung can you sync up branches again? We want to resume to test this. Also, we want to combine in #2570 and #2579 to this pr. @RatkoVasic-NOAA @gspetro-NOAA can you sync up branches in your PRs?

@rhaesung
Copy link
Contributor Author

rhaesung commented Feb 5, 2025

@rhaesung can you sync up branches again? We want to resume to test this. Also, we want to combine in #2570 and #2579 to this pr. @RatkoVasic-NOAA @gspetro-NOAA can you sync up branches in your PRs?

@jkbk2004 Syncing is complete, but the baseline reproduction issue still persists. I'm not sure if you'd like to resume testing on your side

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Feb 5, 2025

@rhaesung can you sync up branches again? We want to resume to test this. Also, we want to combine in #2570 and #2579 to this pr. @RatkoVasic-NOAA @gspetro-NOAA can you sync up branches in your PRs?

@jkbk2004 Syncing is complete, but the baseline reproduction issue still persists. I'm not sure if you'd like to resume testing on your side

@rhaesung ok! then we need to give more time to this pr.

@rhaesung
Copy link
Contributor Author

rhaesung commented Feb 5, 2025

@rhaesung can you sync up branches again? We want to resume to test this. Also, we want to combine in #2570 and #2579 to this pr. @RatkoVasic-NOAA @gspetro-NOAA can you sync up branches in your PRs?

@jkbk2004 Syncing is complete, but the baseline reproduction issue still persists. I'm not sure if you'd like to resume testing on your side

@rhaesung ok! then we need to give more time to this pr.

Thanks @jkbk2004! I'll let you know when it's ready for retesting.

@RatkoVasic-NOAA
Copy link
Collaborator

@rhaesung can you sync up branches again? We want to resume to test this. Also, we want to combine in #2570 and #2579 to this pr. @RatkoVasic-NOAA @gspetro-NOAA can you sync up branches in your PRs?

Done, updated #2579

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Baseline Updates Current baselines will be updated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants