Skip to content

Remove environmental precomputed quantities #3879

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Remove environmental precomputed quantities #3879

wants to merge 8 commits into from

Conversation

costachris
Copy link
Member

@costachris costachris commented Jul 1, 2025 •
edited
Loading

Remove environmental precomputed quantities and lazify

Pending test with ClimaCore level support. Currently, .level(<LAZY>) calls are materialized.

@costachris costachris force-pushed the cc/remove_precomputed_new branch 2 times, most recently from 0107433 to 70c4296 Compare July 9, 2025 15:10
@costachris costachris marked this pull request as ready for review July 9, 2025 15:10
@costachris costachris force-pushed the cc/remove_precomputed_new branch 18 times, most recently from 7b26a96 to 2f06088 Compare July 15, 2025 22:46
@costachris costachris force-pushed the cc/remove_precomputed_new branch from 2f06088 to 1f3b502 Compare July 16, 2025 19:49
tapios and others added 4 commits July 17, 2025 10:57
@tapios @costachris
Added generic draft_sum (as comment for now)
46e774f 
Remove from ᶜρa⁰

Fixed syntax errors

Removed ᶜρ⁰ from precomputed

Remove ᶜq_tot⁰ (and fix forgotten ᶜρa⁰

Introduced helper functions to compute sums over draft, environmental volumetric variables, and specific env variables

Use new helper functions to simplify calculation of ᶜq_tot⁰

Remove redundant draft sum functions

Added specific_env_mse helper

Removed ᶜmse⁰ from precomputed

Remove ᶜq_liq⁰, ᶜq_ice⁰, ᶜq_rai⁰, ᶜq_sno⁰ from precomputed

Remove redundant draft sum helper functions; docstrings

Added TODO

Renamed specific_gs to all_specific_gs; specific_sgs to all_specific_sgs; added docstrings

Added new helper specific_sgs to cleanly extract specific SGS quantities; use it to get env TKE

Change specific_sgs to use ClimaCore>MatrixFIelds for type stability

Removed ᶜspecific (GS precomputed specific quantities); caveat lector [lots of changes]

Correcting errors in previous commit removing gs precomputed quantities

Removed a few more instances of ᶜspecific from precomputed quantities; removed ᶜtke⁰

Missing ᶜtke⁰ removal

Another ᶜtke⁰ fix

Introduced helper function for specific_tke and used it where needed

Corrections of rebasing mistakes; updates to variable_manipulations for clarity.

Removing some more ᶜspecific

Added ᶜtke⁰ computations

Removal of more specifics

Remove h_tot

Syntax corrections

Fixes in cloud fraction

Syntax error fix in jacobian

Remove specific in precomputed_quantities; syntax error corrections
@costachris
debug
c281330
@costachris
restructure env, specific helper functions
2e5bb37
@costachris
lazy thermo vars
ddeeb7b
@costachris costachris force-pushed the cc/remove_precomputed_new branch from 1f3b502 to ddeeb7b Compare July 17, 2025 17:57
@costachris costachris requested a review from tapios July 17, 2025 19:51
akshaysridhar
akshaysridhar reviewed Jul 17, 2025
View reviewed changes
akshaysridhar
akshaysridhar reviewed Jul 17, 2025
View reviewed changes
akshaysridhar
akshaysridhar reviewed Jul 17, 2025
View reviewed changes
src/cache/precomputed_quantities.jl Outdated
return @. lazy(get_ts(ρ, p, θ, e_int, q_tot, q_pt))
end

function thermo_vars(moisture_model, microphysics_model, Y_c, K, Φ)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we're using Y_c, ᶜY, Yc and Y.c to denote the prognostic center valued variables in this PR? Is there a specific convention you'd like us to follow with this lazy broadcast change?

Copy link
Member Author

@costachris costachris Jul 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I think it should be Y for the entire state (since it contains both center and face fields internally), ᶜvar for center fields and ᶜfunc for functions that return lazy broadcasted objects or fields. What's currently called ᶜY, Yc, and Y_c in the code are just Y.c, so I can change those all to be ᶜY for consistency in the parts of the code this PR touches.

@costachris costachris force-pushed the cc/remove_precomputed_new branch 2 times, most recently from 8afe0f3 to da36313 Compare July 17, 2025 22:47
@costachris costachris force-pushed the cc/remove_precomputed_new branch from da36313 to d4ae75d Compare July 17, 2025 23:10
tapios
tapios approved these changes Jul 18, 2025
View reviewed changes
Copy link
Contributor

@tapios tapios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for seeing this to the end!

src/parameterized_tendencies/les_sgs_models/smagorinsky_lilly.jl
Comment on lines +108 to +114
ᶜh_tot = @. lazy(
TD.total_specific_enthalpy(
thermo_params,
ᶜts,
specific(Y.c.ρe_tot, Y.c.ρ),
),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should eventually get rid of the use of the thermodynamic state here and elsewhere. But it's fine for now.

trontrytel
trontrytel reviewed Jul 18, 2025
View reviewed changes
src/utils/variable_manipulations.jl
@@ -42,9 +43,11 @@ Arguments:
- `ρ_fallback`: The grid-mean density used for the fallback value.
- `turbconv_model`: The turbulence convection model, containing parameters for regularization (e.g., `a_half`).
"""
@inline specific(ρχ, ρ) = ρχ / ρ
specific(ρχ, ρ) = ρχ / ρ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the non-lazy version?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as written now, yes because it's still used for broadcasts within function calls
for instance,

 ᶜh_tot = @.  lazy(
        TD.total_specific_enthalpy(
            thermo_params,
            ᶜts,
            specific(Y.c.ρe_tot, Y.c.ρ),
        ),
    )

trontrytel
trontrytel reviewed Jul 18, 2025
View reviewed changes
src/utils/variable_manipulations.jl
"""
ρa⁺(gs)
draft_sum(f, sgsʲs) = mapreduce_with_init(f, +, sgsʲs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whats the performance penalty for pretending we have more than 1 updraft and doing map reduce over 1 element?

trontrytel
trontrytel reviewed Jul 18, 2025
View reviewed changes
trontrytel
trontrytel approved these changes Jul 18, 2025
View reviewed changes
Copy link
Member

@trontrytel trontrytel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@costachris costachris force-pushed the cc/remove_precomputed_new branch from 8304a27 to e5ec6f5 Compare July 21, 2025 15:54
@costachris costachris force-pushed the cc/remove_precomputed_new branch from e5ec6f5 to 97f612f Compare July 22, 2025 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshaysridhar akshaysridhar akshaysridhar left review comments

@trontrytel trontrytel trontrytel approved these changes

@tapios tapios tapios approved these changes

@dennisYatunin dennisYatunin Awaiting requested review from dennisYatunin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@costachris @trontrytel @tapios @akshaysridhar