search

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

Use `ifelse` in `liquid_fraction`, plus a comment about condensate probability #178

Closed glwagner closed 8 months ago

glwagner commented 8 months ago

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[bot] commented 8 months ago

Codecov Report

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

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

Additional details and impacted files ```diff @@ 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 ```

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

glwagner commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

I went ahead and rebased