NRL single-precision changes #797
Conversation
| @@ -1470,8 +1471,11 @@ SUBROUTINE boulac_length(kts,kte,zw,dz,qtke,theta,lb1,lb2) | |||
| dld(iz) = min(dld(iz),zw(iz+1))!not used in PBL anyway, only free atmos | |||
| lb1(iz) = min(dlu(iz),dld(iz)) !minimum | |||
| !JOE-fight floating point errors | |||
| #ifdef SINGLE_PREC | |||
| !JM: keep up the fight, JOE | |||
There was a problem hiding this comment.
Should the #ifdef SINGLE_PREC at line 1474 and the #endif at line 1478 be removed so that dlu and dld are MIN/MAX'd unconditionally, whether or not the code is being compiled single or double floating point precision? I added the #ifdef/#endif to a version of the code where the two lines they enclose had been commented out. The effect of adding #ifdef/#endif was to reinstate them for SINGLE precision; but could this also be a benefit for double precision too?
There was a problem hiding this comment.
@joeolson42 Can you confirm that you want these two lines (bounds for dlu/dld) active regardless of precision?
There was a problem hiding this comment.
I'm in favor of keeping both lines regardless of precision.
There was a problem hiding this comment.
The latest NCAR main already MIN/MAXes these so the #ifdef SINGLE_PREC should not be there.
| @@ -1470,8 +1471,11 @@ SUBROUTINE boulac_length(kts,kte,zw,dz,qtke,theta,lb1,lb2) | |||
| dld(iz) = min(dld(iz),zw(iz+1))!not used in PBL anyway, only free atmos | |||
| lb1(iz) = min(dlu(iz),dld(iz)) !minimum | |||
| !JOE-fight floating point errors | |||
| #ifdef SINGLE_PREC | |||
| !JM: keep up the fight, JOE | |||
There was a problem hiding this comment.
Should the #ifdef SINGLE_PREC at line 1474 and the #endif at line 1478 be removed so that dlu and dld are MIN/MAX'd unconditionally, whether or not the code is being compiled single or double floating point precision? I added the #ifdef/#endif to a version of the code where the two lines they enclose had been commented out. The effect of adding #ifdef/#endif was to reinstate them for SINGLE precision; but could this also be a benefit for double precision too?
| @@ -1733,6 +1733,10 @@ subroutine rswinit & | |||
| tfn = float(i) / float(NTBMX-i) | |||
| tau = bpade * tfn | |||
| exp_tbl(i) = exp( -tau ) | |||
| #ifdef SINGLE_PREC | |||
There was a problem hiding this comment.
Maybe do this unconditionally, single or double precision?
There was a problem hiding this comment.
@michalakes To be clear, this change was required for single precision, but you're wanting to know whether it is OK to do this regardless of precision? If so, I can ask the codeowner for this file.
There was a problem hiding this comment.
Yes, that is correct. The chances of exp_tbl(i) equalling zero and causing a div-zero error are significant lower at double precision, but maybe not impossible.
The WRF version of this routine (in module_ra_rrtmg_swf.F) does this unconditionally.
There was a problem hiding this comment.
The latest NCAR main already MIN/MAXes these so the #ifdef SINGLE_PREC should not be there.
| ! real b,qtmp,rate,qc | ||
| real,external :: xmytw | ||
| ! real(kind=kind_phys) b,qtmp,rate,qc | ||
| ! real(kind=kind_phys),external :: xmytw (now inside the module) |
There was a problem hiding this comment.
No, xmytw is not in a module; it is inside this file outside of a module. Removing the "external" causes it to be undefined and the code fails to compile.
There was a problem hiding this comment.
It looks like the module declaration in my original pull request (https://github.com/NCAR/ccpp-physics/pull/772/files#) didn't make it into the current fork that you're reviewing. The original PR version of this file can be compared from here:
The intent of this and a number of other changes to enclose bare external subroutines into modules was to make it easier to debug the code (especially catching type mismatches) by making subroutine interfaces visible, and I'd recommend keeping them.
There was a problem hiding this comment.
I see you added a lot of "module...end module" in CCPP files that presently use Fortran 77 calling conventions. That seems like a good idea to me, though I'll have to adjust the names slightly to match CCPP's latest file naming standard. (Or rename the files.)
There was a problem hiding this comment.
I was able to get your "module...end module" addition to many files working, though I needed to add "use" statements in more files than you did.
| & cdfun2(nlay,ngptsw) | ||
| real (kind=kind_dbl_prec) :: rand2d(nlay*ngptsw), rand1d(ngptsw) |
There was a problem hiding this comment.
The code fails to compile when these are kind_dbl_prec since the Mersenne Twister code expects default real kind.
There was a problem hiding this comment.
I suggest we use kind_dbl_prec explicitly in mersenne_twister. It does have more than the 23 bits of mantissa precision one can store in a 32-bit floating-point number; it has 32 bits of mantissa. The bulk of the CCPP code assumes default real kind is real(8), so this is consistent with what the code expects.
There was a problem hiding this comment.
I agree the mersenne twister code should be modified to use kind_dbl_prec explicitly instead of relying on compiler flags (-r8) to promote the real type.
There was a problem hiding this comment.
I have applied that change to my branch.
|
I am preparing a replacement for this PR, along with PRs in stochastic-physics, fv3atm, ufs-weather-model, and atmos-cubed-sphere, to get this to work for FV3_RAP in UFS. This will let us include a regression test to ensure UFS development does not break functionality that Neptune requires. |
Sounds good, Sam. Let me know how I can help or if you need me as a reviewer. Thanks for looking into this. |
I think I have what I need for now, but I'll let you know when I need your help again. I certainly will need reviewers for the final product. |
|
This PR is replaced by these: #918 |
DRAFT
TODO: