Use ifelse in liquid_fraction, plus a comment about condensate probability

[glwagner]

This PR makes moves towards resolving #177 by updating liquid_fraction to use ifelse rather than if, else.

I also changed the formatting a bit and added a comment about the condensate probability model that's implemented. The philosophy of the formatting change is to attempt to sharply distinguish between the two ways we have of writing expressions: mathematical notation (symbolic, concise), and in natural language (in this case, english). In my opinion, the current style does not use either concise mathematical notation or natural language, but rather a kind of frankenstein of the two that I find difficult to read (eg verbose, but symbolic).

There are only a few more places that if, else occurs. If these changes are welcome then I'll also fix the others.

Also curious --- there are many annotations of the kind FT <: Real and var::FT. What is the motivation for these? I don't think they speed up compilation. They do serve as a sort of self-documentation for the code though. For example, many functions are diagonalized (many input types are forced to all have type FT), even though the function would likely be valid and work as intended if one or more of the types were an integer, for example.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (20de40f) 92.96% compared to head (2f6ff0f) 93.03%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
+ Coverage   92.96%   93.03%   +0.06%     
==========================================
  Files          10       10              
  Lines        1152     1149       -3     
==========================================
- Hits         1071     1069       -2     
+ Misses         81       80       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[trontrytel]

Thank you!

[glwagner]

Should we do a performance benchmark with ClimaAtmos before merging? If we're concerned about performance that's also a good reason to split this ifelse buisness into a few PRs rather than lumping into one.

[charleskawczynski]

Should we do a performance benchmark with ClimaAtmos before merging? If we're concerned about performance that's also a good reason to split this ifelse buisness into a few PRs rather than lumping into one.

Yeah, I'll add some kernel benchmarks to CI here, and that way we can get a better idea of optimizations without needing to test against ClimaAtmos.

[glwagner]

Should we do a performance benchmark with ClimaAtmos before merging? If we're concerned about performance that's also a good reason to split this ifelse buisness into a few PRs rather than lumping into one.

Yeah, I'll add some kernel benchmarks to CI here, and that way we can get a better idea of optimizations without needing to test against ClimaAtmos.

If you can point me to it, I am happy to run some ClimaAtmos benchmarks before and after this change to report the results. I'd be more comfortable knowing for sure that the changes aren't detrimental (at the least).

[charleskawczynski]

If you can point me to it, I am happy to run some ClimaAtmos benchmarks before and after this change to report the results. I'd be more comfortable knowing for sure that the changes aren't detrimental (at the least).

I would probably suggest just running CI with this branch and look at a few GPU jobs (with moisture).

That said, we're in the middle of updating dependencies, so we'll need to wait until that is resolved. In the mean time, I did add some kernel benchmarks: https://github.com/CliMA/Thermodynamics.jl/blob/main/perf/kernel_bm.jl

[charleskawczynski]

I went ahead and rebased

[charleskawczynski]

I'm fine with merging, the doc string is a nice improvement, thanks @glwagner!