add ccpp_compliant ozphys_2015.f #160
Conversation
mzhangw
requested review from
climbfuji,
grantfirl and
llpcarson
as code owners
physics/ozphys_2015.f
Outdated
| ! June 2015 - Shrinivas Moorthi | ||
| ! | ||
| use machine , only : kind_phys | ||
| use physcons, only : grav => con_g |
There was a problem hiding this comment.
con_g should come in via the argument list, not as a "use physcons" statement. Please correct. This will also require removing ", parameter" for the definition of gravi.
|
So it applies to ozphys_run too? |
|
In theory, yes, but all this old stuff is "grandfathered" in from the first implementation beginning of the year. I think we should try to keep good practices when adding new schemes. If you have time to "fix" ozphys as well, please do so, if not we'll do it in another round of cleanup work. |
|
all comment are addressed |
physics/ozphys.f
Outdated
| character(len=*), intent(out) :: errmsg | ||
| integer, intent(out) :: errflg | ||
| ! | ||
| ! Local variables | ||
| real, parameter :: gravi=1.0/grav | ||
| ! real, parameter :: gravi=1.0/grav |
There was a problem hiding this comment.
I think that you should still define gravi as a real variable (not parameter) and calculate it at the top of the routine (as done in ozphys_2015). Putting 1.0/con_g in the loop potentially hurts performance. By coding as before, by defining gravi and calculating once per call, you're saving (im*levs) division operations.
|
Should I revert ozphys.f to it was since it is a "grandfathered" scheme?
…On Mon, Sep 24, 2018 at 2:32 PM grantfirl ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Looks good to me, although you should probably revert one change in
ozphys_run for performance reasons.
------------------------------
In physics/ozphys.f
<#160 (comment)>:
> character(len=*), intent(out) :: errmsg
integer, intent(out) :: errflg
!
! Local variables
- real, parameter :: gravi=1.0/grav
+! real, parameter :: gravi=1.0/grav
I think that you should still define gravi as a real variable (not
parameter) and calculate it at the top of the routine (as done in
ozphys_2015). Putting 1.0/con_g in the loop potentially hurts performance.
By coding as before, by defining gravi and calculating once per call,
you're saving (im*levs) division operations.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#160 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/Ab87ahADJ_tWEWYXqEZhey23aHtskJu2ks5ueUFvgaJpZM4WoT-k>
.
|
|
Even though ozphys.f is "grandfathered", it is the current operational scheme, and I don't think that we should introduce a code change that degrades performance if we can avoid it. |
|
Please check again. it passed ozphys and ozphys_2015 CCPP RT tests. @grantfirl |
|
Thanks, Man! |
| ! | ||
| ! this code assumes that both prsl and po3 are from bottom to top | ||
| ! as are all other variables | ||
| ! | ||
| use machine , only : kind_phys | ||
| use physcons, only : grav => con_g |
There was a problem hiding this comment.
Thanks for fixing this, Man!
| character(len=*), intent(out) :: errmsg | ||
| integer, intent(out) :: errflg | ||
| ! | ||
| ! Local variables | ||
| real, parameter :: gravi=1.0/grav |
There was a problem hiding this comment.
You could have just removed the ", parameter" statement and used
real :: gravi =1.0/grav
but that's not worth requesting an additional change. Leave it like it is.
* add esmf810 VMEpoch change and iau restart timing change
Resolve various subroutine argument mismatches
add ccpp_compliant ozphys_2015.f