-
Notifications
You must be signed in to change notification settings - Fork 153
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
dtc/hwrf-physics: merge HWRF saSAS with GFS version, update to a more recent version of ccpp-physics from master #433
dtc/hwrf-physics: merge HWRF saSAS with GFS version, update to a more recent version of ccpp-physics from master #433
Conversation
Update my fork with NCAR's master
…pp-physics into coupled_model_ipd_ccpp_b4b
master: regain bit-for-bit identical results between IPD and CCPP for coupled model runs
…06d48e31601912f2cbfe92435c47e' into HEAD
…sasas_into_dtc_hwrf-physics
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.
looks good to me
! | ||
c physical parameters | ||
! parameter(grav=grav,asolfac=0.958) | ||
! parameter(asolfac=0.89) !HWRF |
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.
Why have comment with the HWRF setting? My understanding is that this routine will use whatever asolfac is passed in. Perhaps the comment should mention both asolfac options? Or none?
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 is coped from HWRF codes and also serves as a note to myself when I just started. It may also remind users that we need to change asolfac_deep/shal in NML for difference HWRF/GFS application.
enddo | ||
|
||
else | ||
do i=1,im |
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.
Note for future cleanup: It looks like there is unnecessary redundancy here. Most of these intializations are in common between the HWRF and GFS versions, right? Why not have the initializations that are different for HWRF be distinct from the entire list?
! enddo | ||
! enddo | ||
if (hwrf_samfdeep) then | ||
do i=1,im |
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.
Note for future cleanup: indentation could be made more clear.
bb1 = 4.0 | ||
bb2 = 0.8 | ||
if (hwrf_samfdeep) then | ||
do i = 1, im |
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.
Note for future cleanup: indent loop
@@ -215,10 +216,16 @@ subroutine samfshalcnv_run(im,ix,km,itc,ntc,cliq,cp,cvap, & | |||
fact1 = (cvap-cliq)/rv | |||
fact2 = hvap/rv-fact1*t0c | |||
|
|||
if (.not.hwrf_samfshal) then | |||
cinacrmn=-80. |
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.
Note for future: I'm curious why this initialization was put by itself only for the HWRF version. Was it not used in the GFS version?
@@ -234,7 +241,8 @@ subroutine samfshalcnv_run(im,ix,km,itc,ntc,cliq,cp,cvap, & | |||
c initialize arrays | |||
c | |||
!> - Initialize column-integrated and other single-value-per-column variable arrays. | |||
do i=1,im | |||
if(hwrf_samfshal) then | |||
do i=1,im |
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.
Note for future cleanup: Same comment as in deep scheme; there is redundancy in the initialization sections.
! endif | ||
! enddo | ||
if (hwrf_samfshal) then | ||
do i = 1, im |
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.
Indentation could be clearer.
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.
Looks good. Only minor quibbles for cleanup in the future. The non-HWRF changes looked familiar, and my review concentrated on HWRF changes.
This PR contains
Associated PRs:
#433
NCAR/fv3atm#41
NCAR/ufs-weather-model#39
For regression testing information, see NCAR/ufs-weather-model#39.