Acoustic substepping cleanup: typed AcousticDampingStrategy + new default + BW example#622
Acoustic substepping cleanup: typed AcousticDampingStrategy + new default + BW example#622glwagner wants to merge 19 commits into
Conversation
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Breeze.jl Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.10.
| Benchmark suite | Current: 6ef1a8b | Previous: b0e3e87 | Ratio |
|---|---|---|---|
CBL; Dynamics: anelastic; Microphysics: nothing [Float32]/Compare advections/NVIDIA L4/WENO5 [256, 256, 128] |
106590562.60741785 points/s |
121640055.53819701 points/s |
1.14 |
CBL; Dynamics: anelastic; Microphysics: nothing [Float32]/Advection: WENO5/NVIDIA L4/256x256x128 |
106590562.60741785 points/s |
121640055.53819701 points/s |
1.14 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
This file isn't added to docs/make.jl, is that intentional?
3744152 to
2bc4670
Compare
| nothing, ZDirection(), | ||
| Π⁰, θ⁰, γRᵈ, g, δτ_new) | ||
|
|
||
| @info @sprintf("[S1] b[1] = %.6f, c[1] = %.6e (must be 1.0 and 0.0)", b₁, c₁) |
There was a problem hiding this comment.
Are these prints really necessary? I'm not a fan of printing during tests, if there's any useful information that'd should be tested, not printed. Same applies to all other new tests which print stuff
There was a problem hiding this comment.
probably not. I agree, printing during testing is annoying. Learned that lesson with Oceananigans.
| g = constants.gravitational_acceleration | ||
| Nz = size(grid, 3) | ||
| loc = (nothing, nothing, Center()) | ||
| g Tᵣstants.gravitational_acceleration | ||
| NzTᵣze(grid, 3) | ||
| Tᵣ |
There was a problem hiding this comment.
argh. tried to search and replace
There was a problem hiding this comment.
I reverted 511a875 entirely, I think many things went wrong with that change
Per Giordano's review on PR #622: tests shouldn't print. The diagnostic @info @sprintf("[Sx] ...", value) lines added for debugging substepper behavior had no role beyond noise — every quantity printed has an accompanying @test that does the actual check. Cleared from: - test/substepper_structural.jl (15 prints, plus a dead growth-rate regression block whose only output was an @info) - test/substepper_rest_state.jl (6 prints) - test/atmosphere_model_construction.jl (1 "Testing NaN Checker..." banner) `using Printf` removed from both substepper test files (no remaining sprintf consumers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| function SplitExplicitTimeDiscretization(; substeps = nothing, | ||
| forward_weight = 0.65, | ||
| damping = ThermalDivergenceDamping(coefficient = 0.1), | ||
| sponge = nothing, | ||
| substep_distribution = ProportionalSubsteps()) | ||
|
|
||
| damping isa Union{AcousticDampingStrategy, Tuple} || | ||
| throw(ArgumentError("`damping` must be an `AcousticDampingStrategy` or a tuple of them")) | ||
|
|
||
| sponge isa Union{Nothing, UpperSponge} || | ||
| throw(ArgumentError("`sponge` must be `nothing` or an `UpperSponge`")) | ||
|
|
||
| return SplitExplicitTimeDiscretization(substeps, | ||
| forward_weight, | ||
| divergence_damping_coefficient, | ||
| acoustic_damping_coefficient) | ||
| damping, | ||
| sponge, | ||
| substep_distribution) | ||
| end |
There was a problem hiding this comment.
I just realised #678 conflicts with this PR, but this code has the same problem as what that PR was trying to solve: it's hard to enforce the same float type everywhere. For example running on Metal GPU it's hard to consistently use Float32 everywhere.
There was a problem hiding this comment.
mm yes! We need to add an FT option to the constructor. Is that what you mean?
There was a problem hiding this comment.
Yes, and make sure that all fields have the same floating point type.
| @inline function stage_substep_count_and_size(::ProportionalSubsteps, β_stage, Δt, N) | ||
| Δτ = Δt / N | ||
| Nτ = max(1, round(Int, β_stage * N)) | ||
| return Nτ, Δτ | ||
| end |
There was a problem hiding this comment.
I think it'd be better to take also the grid as argument and do
FT = eltype(grid)
Δτ = FT(Δt) / N
because it's generally more accurate to do the conversion before the division. Same for the other methods below. And then you can replace all the FT(Δτ) -> Δτ
6de3601 to
d92a092
Compare
| [`CompressibleDynamics`](@ref) solves the fully compressible Euler equations with prognostic density ``ρ``. | ||
| This formulation retains acoustic waves and is suitable for problems where full compressibility is important. | ||
| [`CompressibleDynamics`](@ref) solves the fully compressible Euler equations with prognostic | ||
| density ``ρ``. The formulation retains acoustic waves and is suitable for problems where full |
There was a problem hiding this comment.
| density ``ρ``. The formulation retains acoustic waves and is suitable for problems where full | |
| total density ``ρ`` (including dry air, vapor, and condensate). The formulation retains acoustic waves and is suitable for problems where full |
| double-counts the ``\theta`` change from earlier stages (since ``\theta(U^*) = \theta^n + | ||
| \beta_1 \Delta t \, \dot{\theta}^s`` already includes the stage 1 contribution), producing | ||
| an ``O(\Delta t)`` error per time step that causes instability at larger ``\Delta t``. | ||
| Breeze can also fold the vertical component into the column tridiag by setting |
There was a problem hiding this comment.
| Breeze can also fold the vertical component into the column tridiag by setting | |
| This horizontal divergence damping is applied by default. Breeze can also fold the vertical component into the column tridiag by setting |
| Δ(ρu)' &= - γ_h \, ∂_x D_τ , \\ | ||
| Δ(ρv)' &= - γ_h \, ∂_y D_τ . |
There was a problem hiding this comment.
| Δ(ρu)' &= - γ_h \, ∂_x D_τ , \\ | |
| Δ(ρv)' &= - γ_h \, ∂_y D_τ . | |
| Δ(ρu)' &= - γ_x \, ∂_x D_τ , \\ | |
| Δ(ρv)' &= - γ_y \, ∂_y D_τ . |
| The off-centering parameter ``ω = 1/2`` is classical centered Crank–Nicolson — neutrally | ||
| stable for the linearized inviscid system but susceptible to amplification of distributed | ||
| floating-point noise through the non-normal substep operator (see | ||
| [Stability analysis](@ref stability-analysis)). The default ``ω = 0.65`` adds modest | ||
| dissipation; the dimensionless parameter ``ε = 2ω - 1 = 0.3`` quantifies the deviation from | ||
| centered. |
There was a problem hiding this comment.
| The off-centering parameter ``ω = 1/2`` is classical centered Crank–Nicolson — neutrally | |
| stable for the linearized inviscid system but susceptible to amplification of distributed | |
| floating-point noise through the non-normal substep operator (see | |
| [Stability analysis](@ref stability-analysis)). The default ``ω = 0.65`` adds modest | |
| dissipation; the dimensionless parameter ``ε = 2ω - 1 = 0.3`` quantifies the deviation from | |
| centered. | |
| The off-centering parameter ``ω = 1/2`` is classical centered Crank–Nicolson — neutrally | |
| stable for the linearized inviscid system but susceptible to amplification of distributed | |
| floating-point noise through the non-normal substep operator (see | |
| [Stability analysis](@ref stability-analysis)). | |
| A fully implicit backward Euler scheme is obtained with ``ω = 1`` and offers the most dissipation. | |
| The default ``ω = 0.65`` adds modest dissipation; the dimensionless parameter ``ε = 2ω - 1 = 0.3`` quantifies the deviation from centered. |
Summary
Substepper damping infrastructure: typed
AcousticDampingStrategy, two newdamping mechanisms (
UpperSponge,VorticityDamping), an alternativevector-invariant momentum-advection scheme (
CompressibleVectorInvariant),and a constructor-time check that rejects
HydrostaticSphericalCoriolisonnon-hydrostatic dynamics.
What's new in the substepper
SplitExplicitTimeDiscretizationdampingNoDivergenceDamping,ThermalDivergenceDamping,HyperdiffusiveDivergenceDamping,VorticityDamping(new), or aTupleof thesespongeUpperSponge(damping_rate, depth, ramp=CubicRamp())for implicit Rayleigh damping at the lid, folded into the column tridiag (Klemp–Dudhia–Hassiotis 2008 form, as in WRF / MPAS-A)AbstractRampfamily for the sponge profile:CubicRamp(Hermitesmoothstep, default — GPU-cheap),
Sin2Ramp(WRF/MPAS literature form),LinearRamp. Custom shapes via<:AbstractRamp.Vector-invariant momentum advection (FV3/MPAS-style)
CompressibleVectorInvariant <: AbstractAdvectionSchemedecomposes theflux-form momentum advection as
with
(𝐯·∇)ucomputed in vector-invariant form (vorticity flux +Bernoulli + vertical advection) by an inner
WENOVectorInvariant. Pairwith
scalar_advection = WENO():Coriolis safety check
AtmosphereModelnow errors at construction ifHydrostaticSphericalCoriolisis passed: it drops the
2Ω cos(φ)cross-terms that couple horizontalmomentum to
w, and Breeze always carries prognostic ρw. TheSphericalCoriolis(rotation_rate=Ω)non-hydrostatic form is the onlyself-consistent choice on the sphere.
Example
examples/baroclinic_wave.jldrops the explicittimestepperkwarg —AtmosphereModelalready auto-selects:AcousticRungeKutta3forCompressibleDynamics(SplitExplicitTimeDiscretization()).Tests
test/substepper_structural.jl— 17/17 pass (sponge added toget_coefficientsignature; S1 updated to passnothing).test/latitude_longitude_grid.jl— switched fromHydrostaticSphericalCoriolistoSphericalCoriolis.test/acoustic_substepping.jl— passes with newdampingAPI.Note on branch scope
This branch (
glw/hevi-imex-docs) carries a long line of acoustic-substeppercleanup work plus an unrelated older HEVI/IMEX track that's not the focus of
this PR. Reviewers should focus on the recent commits — the typed damping
API,
UpperSponge,VorticityDamping,CompressibleVectorInvariant, andthe
HydrostaticSphericalCoriolisguard.🤖 Generated with Claude Code