CliMA / Thermodynamics.jl

A package containing a library of moist thermodynamic relations.
https://clima.github.io/Thermodynamics.jl/dev/
Apache License 2.0
61 stars 2 forks source link

Fix zero `q_tot` edge case #192

Closed charleskawczynski closed 7 months ago

charleskawczynski commented 7 months ago

This PR fixes a zero q_tot edge case:

using Revise
import Thermodynamics as TD
using CLIMAParameters # load convenience constructors

thermo_params = TD.Parameters.ThermodynamicsParameters(Float32)

e_int=Float32(-3.687397e7);
ρ=Float32(0.0027615533);
q_tot=Float32(0.0);
maxiter=100
tol=Float32(0.003)
ts = TD.PhaseEquil_ρeq(
    thermo_params,
    ρ,
    e_int,
    q_tot,
    maxiter,
    eltype(thermo_params)(tol),
)

fails on main, and passes in this PR.

This was found in the log here: https://buildkite.com/clima/climaatmos-ci/builds/16548. This could be a fix for at least one job breaking in https://github.com/CliMA/ClimaAtmos.jl/issues/2635.

By the way, being able to create parameters in this way is just so nice! (cc @nefrathenrici)

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (1508238) 93.03% compared to head (12e5121) 93.03%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #192 +/- ## ======================================= Coverage 93.03% 93.03% ======================================= Files 10 10 Lines 1149 1149 ======================================= Hits 1069 1069 Misses 80 80 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

szy21 commented 7 months ago

Thanks! Just want to note that if e_int=Float32(-3.687397e7), something else is wrong.

charleskawczynski commented 7 months ago

Yeah, we basically came to that conclusion during the edmf meeting. We’re going to apply one more update: remove the T_min max part so that we can get temperatures below 150 K.

but this does mean that something else is wrong in the RRTMGP job.

maybe it’d be helpful to also print the air temperature assuming all vapor/ice in the SA error?

szy21 commented 7 months ago

Yeah, we basically came to that conclusion during the edmf meeting. We’re going to apply one more update: remove the T_min max part so that we can get temperatures below 150 K.

but this does mean that something else is wrong in the RRTMGP job.

maybe it’d be helpful to also print the air temperature assuming all vapor/ice in the SA error?

That could be helpful I think. The single column all-sky radiation job crashed as well. This is a much simpler case to look at. We should be able to run Float32 and Float64 side by side to see what is going on. I'll see if I can find anything.