Add single precision support for RRTMGP #387
Conversation
|
We may want to try to do https://github.com/CliMA/RRTMGP.jl/blob/9997abad1060ef899e3d8755b5dad9fcaa01c36c/src/optics/GasOptics.jl#L38C33-L38C41 at higher precision. |
sriharshakandala
force-pushed
the
sk/support_single_precision
branch
from
20ae7cb
to
2243cb6
Compare
This does not seem to be the problem! |
sriharshakandala
force-pushed
the
sk/support_single_precision
branch
from
08f7364
to
76c8497
Compare
|
With the above changes, there is a significant reduction in single precision flux errors to less than 1 to 2 W/m^2 compared to double precision results, depending on the test case. However, we are not yet seeing the differences of about 0.1 W/m^2 we are expecting. I need to dig deeper, check if there are any differences in tests or lookup data, compared to the FORTRAN code, since we haven't tracked these in a while! Since these changes do not effect the double precision cases, I was thinking of merging this PR with the current changes and pursue remaining changes a different PR. @simonbyrne @charleskawczynski @szy21 |
|
bors try |
tryBuild succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
|
I'm ok with merging this. @charleskawczynski What do you think? |
|
I’m definitely onboard with incremental changes, is there anything we can/should to to the test suite? (e.g., can any tests be changed from broken to unbroken?) |
Not yet. This change is just to source code. We need to relax the failure tolerance to about |
sriharshakandala
force-pushed
the
sk/support_single_precision
branch
from
438f008
to
12307db
Compare
This involed the following steps: Set k_min for shortwave solver based on floating poing precision Constrain Rdir and Tdir in 2-stream shortwave solver Update tau_threshold and use 3rd order expansion in rte_lw_noscat_source! Update k_min in rte_lw_2stream_source! Update tau_threshold to be precision dependent in rte_lw_2stream_source!
sriharshakandala
force-pushed
the
sk/support_single_precision
branch
from
12307db
to
747f84d
Compare
|
bors r+ |
387: Add single precision support for RRTMGP r=sriharshakandala a=sriharshakandala Add single precision support for RRTMGP Trying: earth-system-radiation/rte-rrtmgp#39 per issue #337 <!--- Review checklist I have: - followed the codebase contribution guide: https://clima.github.io/ClimateMachine.jl/latest/Contributing/ - followed the style guide: https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/ - followed the documentation policy: https://github.com/CliMA/policies/wiki/Documentation-Policy - checked that this PR does not duplicate an open PR. In the Content, I have included - relevant unit tests, and integration tests, - appropriate docstrings on all functions, structs, and modules, and included relevant documentation. --> ---- - [ ] I have read and checked the items on the review checklist. Co-authored-by: sriharshakandala <sriharsha.kvs@gmail.com>
|
Build failed: |
|
bors r+ |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page.
|
| @@ -161,40 +163,44 @@ function rte_lw_2stream_source!( | |||
| # setting references | |||
| (; τ, ssa, g) = op | |||
| (; Rdif, Tdif, lev_source, src_up, src_dn) = src_lw | |||
|
|
|||
| #k_min = FT === Float64 ? FT(1e-12) : FT(1e-4) | |||
| k_min = FT(1e4 * eps(FT)) | |||
There was a problem hiding this comment.
The 1e4 factor seems arbitrary. Is there a principled way to make this a function of eps? Otherwise, maybe a fixed minimum (independent of floating point precision may be better).
There was a problem hiding this comment.
It's from here: https://github.com/earth-system-radiation/rte-rrtmgp/blob/a540749358e8ac15a09c70a7828baf8afd3fa5c9/rte-kernels/accel/mo_rte_solver_kernels.F90#L1106. It seems arbitrary. Maybe we can try something else.
| lw_diff_sec = FT(1.66) | ||
| τ_thresh = sqrt(eps(FT)) |
There was a problem hiding this comment.
It would seem good to use a consistent threshold (eps^(1/4) above, which made sense there given the 3rd-order expansion).
| @inbounds src_up[glay, gcol] = pi * (Zup_top - Rdif[glay, gcol] * Zdn_top - Tdif[glay, gcol] * Zup_bottom) | ||
| @inbounds src_dn[glay, gcol] = | ||
| pi * (Zdn_bottom - Rdif[glay, gcol] * Zup_bottom - Tdif[glay, gcol] * Zdn_top) | ||
| Z = (lev_src_bot - lev_src_top) / (τ_lay * (γ1 + γ2)) |
There was a problem hiding this comment.
If we replace this by a 3rd-order Taylor expansion around tau_lay = 0 when tau_lay < tau_lay, as above, and then use tau_thres = eps^(1/4) as above, perhaps we get more accurate results?
| Zdn_top = -Z + lev_src_top | ||
| Zdn_bottom = -Z + lev_src_bot | ||
| @inbounds src_up[glay, gcol] = pi * (Zup_top - Rdif_lay * Zdn_top - Tdif_lay * Zup_bottom) | ||
| @inbounds src_dn[glay, gcol] = pi * (Zdn_bottom - Rdif_lay * Zup_bottom - Tdif_lay * Zdn_top) | ||
| else |
There was a problem hiding this comment.
As above, perhaps better than using a hard 0 here is to use Taylor expansion for small tau as above.
|
|
||
| coeff = exp(-2 * τ[glay, gcol] * k) | ||
| coeff = exp(-2 * τ_lay * k) | ||
| # Refactored to avoid rounding errors when k, gamma1 are of very different magnitudes | ||
| RT_term = 1 / (k * (1 + coeff) + γ1 * (1 - coeff)) |
There was a problem hiding this comment.
As above, we can use Taylor expansion for 1/k, to 3rd order, and then use k_min = eps^(1/4).
Add single precision support for RRTMGP
Trying: earth-system-radiation/rte-rrtmgp#39 per issue #337