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

Bugfix for convection (prog closure) and gravity wave drag (for stochastic perturbations) needed for GFS/GEFS prototypes #1677

Merged
merged 25 commits into from
Apr 6, 2023

Conversation

lisa-bengtsson
Copy link
Contributor

@lisa-bengtsson lisa-bengtsson commented Mar 24, 2023

Description

Solves issues: #1693 and ufs-community/ccpp-physics#6 (merged in from @mdtoyNOAA PR)

This PR updates the prognostic closure in samfdeepcnv.f and samfshalcnv.f for HR2:

  1. correction (bug-fix) of convective cloud condensate passed to progsigma_calc.F90 routine
  2. Update to only consider positive buoyant levels in integral (to make it general if a convection scheme can have negative buoyant levels between cloud base and cloud top)
  3. Update to make the prognostic closure general to pass in q-tendency due to turbulence from any PBL scheme.
  4. Move out computation of q-tendency due to dynamics from progsigma_clac.F90 (to samfshalcnv and samfdeepcnv) because other convection schemes compute this term in a pre/post step. I will update samf* routines to use pre/post convection in the future.

The impact of these changes on the results are small, two months verification with this update can be seen here (the PR corresponds to the experiment called prog_fix2)
https://www.emc.ncep.noaa.gov/gmb/jhan/vsdbw/progcb/

PR merged in from @mdtoyNOAA:
This PR fixes a bug that occurs when running stochastic physics. As part of ccpp-physics PR#22, the order in which subroutines "gwdps_run" and "drag_suite_run" were called by subroutine "unified_ugwp_run" was reversed. Subroutine "drag_suite_run" is called last, but the variable "RDXZB" (the level of the dividing streamline for blocking) is initialized, which zeros out the value that was calculated in "gwdps_run". RDXZB is needed outside of the unified_ugwp subroutine by stochastic physics. This caused stochastic physics runs to crash. The bug was not caught by regression testing.

The problem has been fixed by changing the declaration of "RDXZB" from "intent(out)" to "intent(inout)" in drag_suite.F90 and drag_suite.meta, and initializing it in drag_suite.F90 only when the GSL drag suite blocking is performed, i.e., when "do_gsl_drag_ls_bl" is equal to .true.

*** This fix needs to be done for running the GEFS.v13 reanalysis and reforecasts.

Top of commit queue on: TBD

Input data additions/changes

  • No changes are expected to input data.
  • There will be new input data.
  • Input data will be updated.

Anticipated changes to regression tests:

  • No changes are expected to any regression test.
  • Changes are expected to the following tests:
    cpld_control_gfsv17 due to updates aimed for HR2.

Subcomponents involved:

  • AQM
  • CDEPS
  • CICE
  • CMEPS
  • CMakeModules
  • FV3
  • GOCART
  • HYCOM
  • MOM6
  • NOAHMP
  • WW3
  • stochastic_physics
  • none

Combined with PR's (If Applicable):

Commit Queue Checklist:

  • Link PR's from all sub-components involved
  • Confirm reviews completed in sub-component PR's
  • Add all appropriate labels to this PR.
  • Run full RT suite on either Hera/Cheyenne with both Intel/GNU compilers
  • Add list of any failed regression tests to "Anticipated changes to regression tests" section.

Linked PR's and Issues:

Testing Day Checklist:

  • This PR is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR.
  • Move new/updated input data on RDHPCS Hera and propagate input data changes to all supported systems.

Testing Log (for CM's):

  • RDHPCS
    • Intel
      • Hera
      • Orion
      • Jet
      • Gaea
      • Cheyenne
    • GNU
      • Hera
      • Cheyenne
  • WCOSS2
    • Dogwood/Cactus
    • Acorn
  • CI
    • Completed
  • opnReqTest
    • N/A
    • Log attached to comment

@lisa-bengtsson lisa-bengtsson changed the title Progc hr2 Correction of convective cloud condensate in prog closure for HR2 Mar 24, 2023
@jkbk2004
Copy link
Collaborator

jkbk2004 commented Apr 4, 2023

@lisa-bengtsson can you sync up branch to resolve conflicts? RegressionTests_hera.intel.log is empty in the branch. It might be easier to attach in the PR (not push into the branch) if hera.intel test is done on your side.

@lisa-bengtsson
Copy link
Contributor Author

The cpld_control_gfsv17 test fails which is unexpected, as the code I'm updating is only exercised at resolutions higher than 30km, and the cpld_control_gfsv17 test is run at C96 resolution. I'm thinking since I am also updating some input/output arguments, the difference with the baseline could be due to some optimization. However, yesterday I ran two tests of cpld_control_gfsv17 with the added -DDEBUG=ON in the compile line. One from the latest "top of develop" ufs-weather-model", and one from this PR, and the output files still differ. Could I expect differences with the baseline due to such updates in arguments passed in/out even with the debug compile flag on? I'm not sure how to proceed. Plotting the differences shows very small diffs, the files are identical after output hour 0.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Apr 4, 2023

The cpld_control_gfsv17 test fails which is unexpected, as the code I'm updating is only exercised at resolutions higher than 30km, and the cpld_control_gfsv17 test is run at C96 resolution. I'm thinking since I am also updating some input/output arguments, the difference with the baseline could be due to some optimization. However, yesterday I ran two tests of cpld_control_gfsv17 with the added -DDEBUG=ON in the compile line. One from the latest "top of develop" ufs-weather-model", and one from this PR, and the output files still differ. Could I expect differences with the baseline due to such updates in arguments passed in/out even with the debug compile flag on? I'm not sure how to proceed. Plotting the differences shows very small diffs, the files are identical after output hour 0.

@lisa-bengtsson if cpld_control_gfsv17 runs thru those subroutines with in/out update, chances are to get the results changed, right?. I think I can try sticking a few print statement in the middle to make sure.

@lisa-bengtsson
Copy link
Contributor Author

The cpld_control_gfsv17 test fails which is unexpected, as the code I'm updating is only exercised at resolutions higher than 30km, and the cpld_control_gfsv17 test is run at C96 resolution. I'm thinking since I am also updating some input/output arguments, the difference with the baseline could be due to some optimization. However, yesterday I ran two tests of cpld_control_gfsv17 with the added -DDEBUG=ON in the compile line. One from the latest "top of develop" ufs-weather-model", and one from this PR, and the output files still differ. Could I expect differences with the baseline due to such updates in arguments passed in/out even with the debug compile flag on? I'm not sure how to proceed. Plotting the differences shows very small diffs, the files are identical after output hour 0.

@lisa-bengtsson if cpld_control_gfsv17 runs thru those subroutines with in/out update, chances are to get the results changed, right?. I think I can try sticking a few print statement in the middle to make sure.

Yes, cpld_control_gfsv17 does run through those subroutines, and I think you are right that chances that updated argument in/out could change results, I just don't really know how to conclude that this is the case. I was hoping it would reproduce with the DEBUG flag on, because then it would point to some optimization. If you could add some print statements that would be fantastic, Hera is unfortunately down today.

@lisa-bengtsson
Copy link
Contributor Author

When Hera comes back up I will run through the operational regression tests.

@BrianCurtis-NOAA
Copy link
Collaborator

Hera is back up.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Apr 4, 2023

@lisa-bengtsson test shows cpld_control_gfsv17 runs thru the code changes on progsigma_calc.f90, samfdeepcnv.f, etc. so, baseline change makes a sense, I guess.

@lisa-bengtsson
Copy link
Contributor Author

The OpnReqTest for cpld_control_gfsv17_hera.intel.log passes:

OpnReqTests_cpld_control_gfsv17_hera.intel.log

@lisa-bengtsson
Copy link
Contributor Author

@lisa-bengtsson test shows cpld_control_gfsv17 runs thru the code changes on progsigma_calc.f90, samfdeepcnv.f, etc. so, baseline change makes a sense, I guess.

Yes, I think you are right, since the ORT's passes I also believe it is OK. I will update the description of this PR to also create new baselines for the cpld_control_gfsv17 test.

@grantfirl grantfirl mentioned this pull request Apr 5, 2023
6 tasks
@lisa-bengtsson
Copy link
Contributor Author

@jkbk2004 I updated to the top of develop and then merged in the gravity wave drag PR in ccpp/physics. Please let me know the next step.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Apr 5, 2023

@mdtoyNOAA #1696 is combined into this pr. Please, go ahead to confirm. @lisa-bengtsson Can you update the PR title to reflect #1696?

@lisa-bengtsson lisa-bengtsson changed the title Correction of convective cloud condensate in prog closure for HR2 Bugfix for convection (prog closure) and gravity wave drag (for stochastic perturbations) needed for GFS/GEFS prototypes Apr 5, 2023
@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. jenkins-ci Jenkins CI: ORT build/test on docker container labels Apr 5, 2023
@BrianCurtis-NOAA
Copy link
Collaborator

Has both GNU and Intel tests been run on Hera or Cheyenne?

@jkbk2004 jkbk2004 requested a review from Qingfu-Liu April 6, 2023 12:44
@jkbk2004
Copy link
Collaborator

jkbk2004 commented Apr 6, 2023

All test are done. we can start merging process.

DusanJovic-NOAA
DusanJovic-NOAA previously approved these changes Apr 6, 2023
@BrianCurtis-NOAA
Copy link
Collaborator

Once the commit to update hash and .gitmodules comes in it will override Dusan's approval. What I do is wait for that final commit and then request final approvals.

@lisa-bengtsson
Copy link
Contributor Author

Sorry @BrianCurtis-NOAA it seems that I pushed old logfiles when updating .gitmodules, can I revert that somehow? So sorry.

@BrianCurtis-NOAA
Copy link
Collaborator

@lisa-bengtsson Actually it looks like you were fine until you force-pushed. Unfortunately the force push reverted log files and the BL_DATE in rt.sh to an old value.

@lisa-bengtsson
Copy link
Contributor Author

oh no, there was a conflict with rt.sh and the logfiles with my branch as they were pushed from others and I screwed up. Can I revert to the last successful commit and try to update .gitmodules again?

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Apr 6, 2023

@lisa-bengtsson it seems it doesn't recover fully with git revert 68aa. I think I have a backup local files ok for logs. Do you want me to push the logs again and fix with new bl_date?

@lisa-bengtsson
Copy link
Contributor Author

@jkbk2004 yes, thank you. So sorry about the trouble.

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Apr 6, 2023

@lisa-bengtsson sorry I was in meeting. the fv3 pr was merged. can you update fv3 pointer? correct one is NOAA-EMC/fv3atm@decb0b9

@jkbk2004
Copy link
Collaborator

jkbk2004 commented Apr 6, 2023

@DeniseWorthen @BrianCurtis-NOAA sounds like all set. Please, go head for final reviews and approvals.

@jkbk2004 jkbk2004 merged commit b6146cd into ufs-community:develop Apr 6, 2023
@DeniseWorthen
Copy link
Collaborator

Tag EP4 has been created

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. jenkins-ci Jenkins CI: ORT build/test on docker container Ready for Commit Queue The PR is ready for the Commit Queue. All checkboxes in PR template have been checked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants