-
Notifications
You must be signed in to change notification settings - Fork 148
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
Update Tiedtke scheme with MPASv8.0.0 updates #1025
base: main
Are you sure you want to change the base?
Conversation
@matusmartini Have these changes been tested and evaluated for scientific impact in any UFS application? |
@lisa-bengtsson This scheme is orphaned in CCPP, and it sounds as if there is an interest to try it with FV3? We tested with NEPTUNE at various configurations and found improved skill especially in lower troposphere. More info and discussion on this is in #1024 |
@matusmartini thank you for pointing me to the discussion. |
physics/cu_ntiedtke.F90
Outdated
real(kind=kind_phys),private :: rovcp,r5alvcp,r5alscp,ralvdcp,ralsdcp,ralfdcp | ||
|
||
real(kind=kind_phys),parameter:: t13 = 0.333333333 | ||
real(kind=kind_phys),parameter:: tmelt = 273.16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temperature_at_zero_celsius
and triple_point_temperature_of_water
are both defined as host-defined constants already. For consistency across schemes and hosts, tmelt
should probably be an argument with one of the above variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Maybe this could be a separate PR?
physics/cu_ntiedtke.F90
Outdated
@@ -14,27 +14,29 @@ module cu_ntiedtke | |||
! this also requires redefining derived constants in the | |||
! parameter section below | |||
use physcons, only:rd=>con_rd, rv=>con_rv, g=>con_g, & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be passed in through the argument list (standard names that should be passed in are given below in parentheses):
con_rd
=gas_constant_of_dry_aircon_rv
=gas_constant_water_vaporcon_g
=gravitational_accelerationcon_cp
=specific_heat_of_dry_air_at_constant_pressurecon_hvap
=latent_heat_of_vaporization_of_water_at_0Ccon_hfus
=latent_heat_of_fusion_of_water_at_0C
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Maybe this could be a separate PR?
physics/cu_ntiedtke.F90
Outdated
logical :: lmfpen,lmfmid,lmfscv,lmfdd,lmfdudv | ||
parameter(lmfpen=.true.,lmfmid=.true.,lmfscv=.true.,lmfdd=.true.,lmfdudv=.true.) | ||
!-------------------- | ||
logical,parameter:: lmfpen = .true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should any of these logicals be settable through namelist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a nice feature.
physics/cu_ntiedtke.F90
Outdated
real(kind=kind_phys), dimension( :, : ), intent(in ) :: pzz, prsi | ||
!--- input arguments: | ||
integer, intent(in):: lq,km,ktrac | ||
integer,intent(in),dimension(lq):: lmask |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument variable arrays should be assumed shape for the CCPP. See https://ccpp-techdoc.readthedocs.io/en/latest/CompliantPhysicsParams.html#input-output-variable-argument-rules.
physics/cu_ntiedtke.F90
Outdated
! end if | ||
deallocate(pcen) | ||
deallocate(ptenc) | ||
errmsg = 'cu_ntiedtke_run OK' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary to set errmsg
since it isn't checked unless errflg /= 0
(at least in UFS-based models).
physics/cu_ntiedtke.F90
Outdated
|
||
return | ||
end subroutine cudlfsn | ||
ike=klev-3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These large sections where there are whitespace changes are really difficult to detect changes. I wonder if we could see a version without whitespace changes for review, then reinstate them after review?
According to the discussion in the 8/2/2023 CCPP Physics Code Mgmt meeting, we will delay the final review of this PR until Wei Wang (who did the Tiedtke updates for MPAS v8) is back in mid September 2023. |
Thank you @ligiabernardet for the discussion yesterday and @grantfirl for all of the above comments. The goal of my initial commit was to point out the differences between orphaned Tiedtke scheme in CCPP and seemingly cleaner version in MPAS with the scientific updates without imposing sense of urgency. These changes simply attempt to sync as much as possible with MPAS version while leaving MPAS-specifics out (for example units of latent and sensible heat, units of pressure, humidity). If the ultimate goal is to have ONE Tiedtke scheme across MPAS and CCPP then some of @grantfirl suggestions (such as the one about assumed shape of argument variable arrays) would have to propagate into MPAS, bridging the two... In addition, it would be nice to avoid duplicate efforts with MPAS shared physics driver @dustinswales going forward. In the interim, proceeding with these changes (after review) would bring the CCPP version closer to the MPAS version. Alternatively, I could just bring the very minimal set of updates and these could just be the scientific ones and bugfixes. Perhaps that would be a better start, when @weiwangncar is available, and would also make the review much easier if there is preference for that. @yangfanglin |
May I ask what the status of this PR is? |
Is there a status update on this PR? |
@climbfuji @areinecke I don't think there is anything "technical" holding up this PR and I'd like to see these changes propagate into the codebase. I think the initial holdup was testing, but since this scheme is not used/tested by any UFS/SCM applications, my inclination is to rely on the organization that has tested it (e.g NRL). In general, when an untested and orphaned scheme is adopted, what burden do we put on the contributor beyond testing/validating in their host? I view this as a great contribution and I don't think it makes sense for NRL to put together a SDF for the SCM/UFS testing. In the future, if a UWM application wants to use this scheme, they will need to build the infrastructure and validate at that time. @lisa-bengtsson @yangfanglin Do you have objections to moving forward? |
@dustinswales I don't have any objections about the code changes. I do know that the HAFS developers have done a lot of tests with the Tiedke scheme as they want diversity in their A and B configurations. It could be good to check with Zhan Zhang at EMC if there's any concern for their testing for HAVSv3. |
Thank you for following up on this. Before this is merged, I need to resolve the conflicts and add one more commit with improved comments (thanks to Michael Diaz from NRL). |
@dustinswales : HAFS will not use Tiedtke for the upcoming implementation. No objection to committing this PR. It would be desirable to set up a HAFS RT after the PR is completed ( @AndrewHazelton ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look fine to me.
@yangfanglin I opened an issue in the UWM to create a Tiedtke based RT for HAFS. ufs-community/ufs-weather-model#2542
The updates include: introducing scale dependency factors (Wang 2022, WAF), updated entrainment coefficients, vertical indexing fix, and horizontal wind tendency term (pgf_u, pg_v) calculation also for shallow convection not just deep and mid- convection, cleaner fields declarations.
NEW: Added comments from IFS documentation. Thanks to Michael Diaz from NRL for the updates and tuning experiments.(00d04d5)
Issue #1024