Skip to content

Defuse advection kernels#4270

Queued
imreddyTeja wants to merge 1 commit intomainfrom
tr/defuse
Queued

Defuse advection kernels#4270
imreddyTeja wants to merge 1 commit intomainfrom
tr/defuse

Conversation

@imreddyTeja
Copy link
Member

@imreddyTeja imreddyTeja commented Feb 4, 2026

Purpose

I took a look at a prognostic EDMF simulation with 1M microphysics and noticed that there are some easy performance improvements in the advection code (at the cost of readability)

To-do

  • Unrelated to this PR, but I need to document guidelines on code patterns for atmos developers to avoid for performance reasons

Content

  • fix inaccurate comment in update_jacobian
  • cache expensive ᶠwinterp_matrix(ᶜJ * ᶜρ) ⋅ DiagonalMatrixRow(g³ʰ(ᶜgⁱʲ)) in update_jacobian
  • change updraft_sedimentation to an in-place eager computation
  • defuses advection code

With the following config options, this PR improves SYPD from 0.11 to 0.124

aerosol_radiation: true
approximate_linear_solve_iters: 2
cloud_model: "quadrature"
dt_cloud_fraction: "1hours"
dt_save_state_to_disk: "30days"
dz_bottom: 30.0
edmfx_detr_model: "Generalized"
edmfx_entr_model: "Generalized"
edmfx_filter: true
edmfx_nh_pressure: true
edmfx_sgs_diffusive_flux: true
edmfx_sgs_mass_flux: true
edmfx_upwinding: "first_order"
edmfx_vertical_diffusion: true
h_elem: 16
implicit_diffusion: true
implicit_sgs_advection: true
implicit_sgs_entr_detr: true
implicit_sgs_mass_flux: true
implicit_sgs_nh_pressure: true
implicit_sgs_vertdiff: true
insolation: "timevarying"
max_newton_iters_ode: 3
moist: "nonequil"
netcdf_output_at_levels: true
precip_model: "1M"
prescribed_aerosols: ["CB1", "CB2", "DST01", "DST02", "DST03", "DST04", "DST05", "OC1", "OC2", "SO4", "SSLT01", "SSLT02", "SSLT03", "SSLT04", "SSLT05"]
prognostic_tke: true
rad: "allskywithclear"
rayleigh_sponge: true
t_end: "120days"
time_varying_trace_gases: ["CO2", "O3"]
toml: [toml/longrun_aquaplanet_diagedmf.toml]
turbconv: "prognostic_edmfx"
viscous_sponge: true
z_elem: 63
z_max: 60000.0

FLOAT_TYPE: "Float32"
albedo_model: "CouplerAlbedo"
atmos_config_file: "config/atmos_configs/climaatmos_progedmf_1m.yml"
checkpoint_dt: "366days"
coupler_toml: ["toml/amip_progedmf_1m.toml"]
dt: "30secs"
dt_cpl: "30secs"
dt_rad: "600secs"
energy_check: false
land_model: "integrated"
mode_name: "amip"
radiation_reset_rng_seed: true
start_date: "20100101"
surface_setup: "PrescribedSurface"
topo_smoothing: true
topography: "Earth"

  • I have read and checked the items on the review checklist.

@imreddyTeja imreddyTeja force-pushed the tr/defuse branch 7 times, most recently from 4a72738 to 9344f69 Compare February 4, 2026 23:36
),
))

ᶜinv_ρ̂ = p.scratch.ᶜtemp_scalar_6
Copy link
Member

Choose a reason for hiding this comment

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

You can probably use ᶜtemp_scalar_4 here and ᶜtemp_scalar_5 for vtt?

ᶜw = MatrixFields.get_field(p.precomputed, w_name)
vtt = @.lazy(
vtt = p.scratch.ᶜtemp_scalar_2
@. p.scratch.ᶜtemp_scalar_6 = specific(ᶜρq, Y.c.ρ)
Copy link
Member

Choose a reason for hiding this comment

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

Use temp_scalar_3?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe more readable if you do something like:

ᶠρ = p.scratch.ᶠtemp_scalar
@. p.scratch.ᶠtemp_scalar = ᶠinterp(Y.c.ρ * ᶜJ) / ᶠJ
ᶜq = p.scratch.ᶜtemp_scalar_2
@. ᶜq = specific(ᶜρq, Y.c.ρ)

and use those variables?

Comment on lines 74 to 78
@. p.scratch.ᶜtemp_scalar_6 =
-(ᶜw) * p.scratch.ᶜtemp_scalar_6 * (e_int_func(thp, ᶜT) + ᶜΦ + $(Kin(ᶜw, ᶜu)))
@. Yₜ.c.ρe_tot -= ᶜprecipdivᵥ(
ᶠinterp(Y.c.ρ * ᶜJ) / ᶠJ * ᶠright_bias(
Geometry.WVector(-(ᶜw)) * specific(ᶜρq, Y.c.ρ) *
(e_int_func(thp, ᶜT) + ᶜΦ + $(Kin(ᶜw, ᶜu))),
p.scratch.ᶠtemp_scalar * ᶠright_bias(
Geometry.WVector(-(ᶜw)p.scratch.ᶜtemp_scalar_6),
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look right here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, that is a typo

Copy link
Member

@szy21 szy21 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks fine to me. Maybe it would be good if @sajjadazimi takes a look as well.

Copy link
Member

@sajjadazimi sajjadazimi 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. I just found a minor issue in water advection.

Comment on lines 123 to 131
(e_int_func(thp, ᶜT⁰) - e_int_func(thp, ᶜT) - $(Kin(ᶜw, ᶜu))),
p.scratch.ᶜtemp_scalar_2,
Copy link
Member

Choose a reason for hiding this comment

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

This is not equal to temp_scalar_2. Here instead of T^j we should use T^0

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, good catch!

Copy link
Member

Choose a reason for hiding this comment

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

our regression test on 1M is useless

Copy link
Member Author

Choose a reason for hiding this comment

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

😳 good catch, I'll fix this and double check the other changes

@imreddyTeja imreddyTeja added this pull request to the merge queue Feb 13, 2026
Any commits made after this event will not be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants