-
Notifications
You must be signed in to change notification settings - Fork 9
fix: remove allocations in P3 scheme #621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This change is part of the following stack: Change managed by git-spice. |
a656b96
to
b95a0b7
Compare
465789a
to
f246c9a
Compare
b95a0b7
to
b1b7058
Compare
f246c9a
to
34838bc
Compare
b1b7058
to
5d5af66
Compare
6e14234
to
07f1c02
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #621 +/- ##
==========================================
+ Coverage 91.65% 92.79% +1.14%
==========================================
Files 48 48
Lines 1953 1957 +4
==========================================
+ Hits 1790 1816 +26
+ Misses 163 141 -22
🚀 New features to boost your workflow:
|
5d5af66
to
f024268
Compare
f024268
to
37b9712
Compare
c96aaf3
to
f7364a3
Compare
@@ -337,7 +338,7 @@ Needed for numerical integrals in the P3 scheme. | |||
function particle_terminal_velocity(velocity_params::CMP.TerminalVelocityType, ρs...) | |||
(ai, bi, ci) = Chen2022_vel_coeffs(velocity_params, ρs...) | |||
v_terms = Chen2022_monodisperse_pdf.(ai, bi, ci) # tuple of functions | |||
v_term(D) = sum(@. D |> v_terms) | |||
v_term(D) = unrolled_sum(v_term -> v_term(D), v_terms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding UnrolledUtilities
and the unrolled_sum
statement is needed for inference on julia 1.10 only. This is a small package with no dependencies, so it should be fine. Further, it is a dependency of ClimaCore, so the addition doesn't matter from the perspective of ClimaAtmos/KiD anyways. If we ever drop support for julia 1.10, then this could potentially be revisited.
ref_val = if FT == Float64 | ||
FT(0.0002736160475969029) | ||
else | ||
# loss of precision due to | ||
# `exp(-B * V_l_converted * Δt * exp(a * Tₛ))` -> 0.9999999 (Float32) | ||
# instead of 0.9999998631919762 (Float64). | ||
FT(0.00023841858f0) | ||
end | ||
TT.@test Array(output)[1] ≈ ref_val |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This arises from fixing the type instability here
f7364a3
to
f834e76
Compare
Purpose
This pull request introduces several improvements and refactorings to the P3 microphysics scheme and its related utilities, focusing on code clarity, numerical stability, and performance benchmarking. The most significant changes are grouped below by theme.
P3 Scheme Refactoring and Utility Improvements
get_bounded_thresholds
utility to consistently clamp and sanitize particle size thresholds, and refactored related functions (integral_bounds
,get_segments
) to use this for more robust interval handling. This improves the reliability of segment and bounds calculations for particle distributions. [1] [2]ice_mass
,ice_density
,get_∂mass_∂D_coeffs
,∂ice_mass_∂D
,ϕᵢ
) to accept explicit arguments or state objects rather than variadic arguments, improving type stability and readability. [1] [2] [3] [4]Numerical Integration and Terminal Velocity
unrolled_sum
for efficient summation inparticle_terminal_velocity
, enhancing both correctness and performance. [1] [2] [3] [4]Consistency and Readability Updates
1000
and1_000_000
instead of scientific notation). [1] [2]Testing and Benchmarking Enhancements
These changes collectively improve the robustness, maintainability, and test coverage of the P3 microphysics codebase.