Force fractional exponent arguments to be 0 minimum #841
Conversation
| @@ -42,7 +42,7 @@ which is related to the total soil porosity (`ν`) and | |||
| volumetric soil water content (`θ_w = θ_l+θ_i`). | |||
| """ | |||
| function volumetric_air_content(θ_w::FT, ν::FT) where {FT} | |||
| θ_a = ν - θ_w | |||
| θ_a = max(ν - θ_w, eps(FT)) | |||
There was a problem hiding this comment.
it's possible this could happen and be OK, for example, if saturated soil freezes, theta_w > \nu because ice has a bigger volume than water does?
| @@ -25,7 +25,7 @@ function microbe_source( | |||
| Vmax = α_sx * exp(-Ea_sx / (R * T_soil)) # Maximum potential rate of respiration | |||
| Sx = p_sx * Csom * D_liq * θ_l^3 # All soluble substrate, kgC m⁻³ | |||
| MM_sx = Sx / (kM_sx + Sx) # Availability of substrate factor, 0-1 | |||
| O2 = D_oa * O2_a * ((ν - θ_l)^(FT(4 / 3))) # Oxygen concentration | |||
| O2 = D_oa * O2_a * (max((ν - θ_l), 0)^(FT(4 / 3))) # Oxygen concentration | |||
There was a problem hiding this comment.
I think here it would be good to understand when this type of error happens... as we discussed we dont think it should happen ever
There was a problem hiding this comment.
is it just roundoff? or is nu not the same as the soil nu?
There was a problem hiding this comment.
it should be the same? we should look into it
| @@ -82,7 +82,7 @@ function co2_diffusivity( | |||
| T_ref = FT(LP.T_0(earth_param_set)) | |||
| P_ref = FT(LP.P_ref(earth_param_set)) | |||
| θ_a = volumetric_air_content(θ_w, ν) | |||
| D0 = D_ref * (T_soil / T_ref)^FT(1.75) * (P_ref / P_sfc) | |||
| D0 = D_ref * max((T_soil / T_ref), 0)^FT(1.75) * (P_ref / P_sfc) | |||
There was a problem hiding this comment.
This one is a true error, often caused by instability. On GPU this would give NaNs, on CPU it will break...
maybe here is another place where we should break (or return NaN?) and give a good error message, not keep going?
kmdeck
force-pushed
the
ar/fractionalexp_0max
branch
from
84a9170
to
d544c00
Compare
|
@kmdeck I think we should add error messages, possibly break or not for GPU, and then merge this PR? (happy to discuss options) |
|
I think there may be other issues - I am trying to run a regional sim on the cluster right now where we get NaNs to look at more granular output. |
This commit ensure co2_parameterizations fractional exponentiation arguments are not negative, because if it is, it returns an error.
This is a PR to test if this fixes long_run NaNs for soil co2 (which would also error in cpu simulations).
We need to be warry, that those arguments should not be negative anyway, so if they are there is probably an issue somewhere else in the code or instability.
While it is helpful to ensure the code doesn't break, we should make sure we catch those issue, so if we push this PR to main, we should add a warning when those arguments are set to 0.