Rename the Breeze seam for consistency: base_pressure and ρE - #698
Merged
Merged
Conversation
`atmosphere_model` passed its `surface_pressure` keyword — the scalar it also hands to `CompressibleDynamics` as `base_pressure`, the reference atmosphere's pressure at z = 0 — into `HydrostaticallyBalancedDensity`, whose keyword is a per-column ground-pressure override applied to every column. Since Breeze#893 those are different quantities: the reference carries the datum reduced hydrostatically to each column's bottom face, and `HydrostaticallyBalancedDensity()` anchors on that by default so the cold start agrees with the reference it is differenced against. On the flat z = 0 grids the coupled examples use, the two anchors are equal exactly (`columnwise_surface_pressure_field` returns the datum when the bottom face is at z = 0), so this is a no-op there. On a raised or terrain-following grid the override re-creates the O(ρgh) mismatch between state and reference that #893 fixed. It is also wrong whenever a caller passes its own `dynamics` with a different `base_pressure`: the density was then anchored at the keyword's value rather than the reference's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Since Breeze#893, `surface_pressure` in Breeze is the pressure at the ground — a 2D field, per column on terrain — and the scalar pressure of the reference atmosphere at z = 0 is `base_pressure`. NumericalEarth's keyword forwards to the latter; #689 translated it at the `CompressibleDynamics` call and kept the old spelling to limit the blast radius. That left NumericalEarth with the name #893 rejected on the quantity it rejected it for: over terrain the two differ by O(ρgh), and the nested default is the domain-mean ERA5 mean sea level pressure, not the surface pressure. `base_pressure` rather than `sea_level_pressure` because the keyword is a pure pass-through, so the reader meets the same word in Breeze's docstrings and in its rejection message for the old spelling; and because `:sea_level_pressure` in NumericalEarth already names a time-varying 2D dataset field, not a scalar datum. Untouched, because they are the pressure at the ground and so correctly named: ERA5 `:surface_pressure` (`sp`), and `hydrostatic_pressure_from_surface(T, surface_pressure, orography)`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The field the coupler writes sensible heat into is called `Jᵉ`, and
`net_fluxes` returns it under the key `ρe`. Breeze reserves `e` for
turbulent kinetic energy and uses `E` for total energy: notation.md:52
("`e` is reserved for turbulent kinetic energy"), :54 (`E` is total
energy and the formulation-agnostic energy key), :167 (`e` is subgrid
TKE). So the spelling says TKE where the quantity is enthalpy.
Nothing was misrouted — the boundary condition is installed under
`energy_bc_key()`, which #689 already pointed at `total_energy_density_name`
(`:ρE`). But `ρe` is now a live prognostic whenever the TKE closure is
on, which a downstream nested configuration enables by default, so
`net.ρe = Qc` sits twenty lines from a genuine `ρe` tracer meaning
something else. That is the collision class of Breeze#956, where the
`e`→`s` rename left stale keys silently accepted.
Internal, so patch-releasable: `Jᵉ` is a local binding, and `ρe` is a
NamedTuple key with one producer (`net_fluxes(::BreezeAtmosphere)`) and
one consumer (`_assemble_net_atmosphere_fluxes!`) in the same file.
`net_fluxes` is not exported from NumericalEarth, and there is no
cross-component schema to honor — ocean, sea ice and Veros each return
their own shape or `nothing`. No test asserts the key, and no known
downstream caller references it.
The four `Jᵉ` in `dry_layer_humidity.jl` are a different quantity — a
vapor flux where the superscript marks the evaporating surface — and are
deliberately untouched. Do not do this rename with a global substitution.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D2fuRfqFoMFjvYyx3idQtQ
Pins the invariant restored by "Anchor the coupled cold start on the reference, not the datum": `atmosphere_model`'s resting initialization must anchor the density on the reference state's per-column ground pressure, not on the z = 0 datum, which is the same scalar and equal to it only when the domain bottom sits at z = 0. Nothing in-tree exercised that path — both coupled examples are bottomed at z = 0, where the two anchors coincide, and the nested constructor passes `initialize = false` — so without this the fix has no guard and a reinstated override would keep CI green. The second assertion earns its place because both quantities anchored at the datum would also agree with each other: it pins that the reference is genuinely reduced to the domain bottom. On this branch: 2/2. Reverted to the pre-fix anchor, both fail — `max|p - pᵣ|` is 22020.6 Pa against a 1e-6 tolerance, and the bottom-cell pressure is 100119.5 Pa where the reduced value is under 91192.5. It lands after the keyword rename because it calls `base_pressure`, which does not exist before it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ewquon
force-pushed
the
eq/base-pressure-keyword
branch
from
September 22, 2026 17:32
020736b to
e760f10
Compare
glwagner
approved these changes
Sep 24, 2026
glwagner
reviewed
Sep 24, 2026
Member
There was a problem hiding this comment.
what are these additional tests doing? seems like the PR is just renaming
Collaborator
Author
There was a problem hiding this comment.
Verifying that our usage of pressure here is consistent with the updated usage in Breeze
`nested_atmosphere_model` no longer takes `surface_pressure`; the keyword falls through `kw...` into `atmosphere_model`, whose keyword list is closed, so the call raises a `MethodError` rather than being ignored. The testset arrived from main after the rename was written and merged without a textual conflict, so neither side could flag it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AWWn9XNLXVywosTVY8uS9z
`maximum(abs, p .- pᵣ) < 1e-6` is an absolute bound on a 10^5 Pa quantity, so it carries no margin under a narrower float type. `≈` scales with the element type and still separates the two anchors by three orders of magnitude in Float32 and seven in Float64, against a difference of 22 kPa. `p[:, :, 1]` selects the bottom level whatever extent the Flat dimension has. The comment now states what the two names on screen mean; the rest of what it said is in this PR's description. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AWWn9XNLXVywosTVY8uS9z
`@test p ≈ pᵣ` cannot run: the state pressure is (8, 1, 20) while the reference is a single broadcast column, (1, 1, 20), so `isapprox` reaches non-broadcasting `-` and throws DimensionMismatch. The reference being one column is the point — it is horizontally uniform on a height- coordinate grid, and the assertion is that every column of the cold start matches it. `all(p .≈ pᵣ)` keeps the relative tolerance the absolute 1e-6 bound was replaced for — the concern was a fixed bound on a 1e5 Pa quantity under a narrower float type — and compares elementwise, which broadcasts. Verified both directions: 2/2 here, and against the pre-fix anchor both assertions fail, the second evaluating 100119.5 < 91192.5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NumericalEarth's Breeze-facing extension still writes two names that Breeze has since redefined, so the seam says one thing and means another. Both are renames with no behavior change; a third commit is the behavior prerequisite that makes the first rename honest.
The reference datum. Rename
surface_pressure→base_pressureonatmosphere_model,atmosphere_simulation, bothnested_atmosphere_modelmethods, andnested_atmosphere_simulation.Breeze#893 split
surface_pressureintobase_pressure(the reference atmosphere's pressure at z = 0, a scalar) andsurface_pressure(the pressure at the ground, a 2D field, per column on terrain). #689 adopted the split at theCompressibleDynamicscall but kept NumericalEarth's own keyword spelledsurface_pressureto limit the blast radius. That left the worse name on the wrong quantity — the keyword forwards to the z = 0 datum, and the nested default is the domain-mean ERA5 mean sea level pressure.The prerequisite. Do not pass a
surface_pressureoverride toHydrostaticallyBalancedDensity.atmosphere_modelpassed the same scalar it hands toCompressibleDynamicsasbase_pressureintoHydrostaticallyBalancedDensity(; surface_pressure), whose keyword is a per-column ground-pressure override. Renaming the keyword while it still fed both slots would putbase_pressureon a call that wants a surface pressure, so this lands first: the marker now takes its default, anchoring on the reference's per-column ground pressure.Nothing in-tree changes: both coupled examples sit on grids bottomed at z = 0, where the two anchors are equal exactly, and the nested path passes
initialize = false, so it never reachesset!(model; θ = potential_temperature, ρ = HydrostaticallyBalancedDensity()). The fix affects a directatmosphere_modelcall on a raised or terrain-following grid, where the override re-creates the O(ρgh) state–reference mismatch that #893 fixed.The energy coupling flux. Rename
Jᵉ→Jᴱ,net.ρe→net.ρE.Breeze reserves
efor turbulent kinetic energy and usesEfor total energy, including as the formulation-agnostic energy key (notation.md:52, :54, :167). The coupler's field wasJᵉandnet_fluxesreturned it underρe. Nothing was misrouted — the boundary condition is installed underenergy_bc_key(), which #689 already pointed attotal_energy_density_name— butρeis a live TKE prognostic whenever the TKE closure is on, sonet.ρe = Qcsat twenty lines from a genuineρetracer meaning something else. Internal:Jᵉis a local binding andρeis a NamedTuple key with one producer and one consumer in the same file;net_fluxesis not exported and there is no cross-component schema. The fourJᵉindry_layer_humidity.jlare a different quantity — a vapor flux whose superscript marks the evaporating surface — and are deliberately untouched.Breaking change
Only the datum rename is breaking, for callers passing
surface_pressure=to those constructors; the one in-tree caller (test/test_nested_simulation.jl) is updated. A stale keyword throws aMethodErrornaming it, at the NumericalEarth boundary rather than inside Breeze. The energy rename is internal and patch-safe.Testing
CPU against the branch head (Breeze 0.11.2, Oceananigans 0.113.0, ClimaSeaIce 0.5.10):
test_nested_simulation281/281,test_breeze_coupling44 pass / 2 broken / 0 fail (the 2@test_brokenare the pre-existing baseline),test_breeze_dry_layer_land19/19. The nested suite covers the renamed keyword; the coupling and dry-layer-land suites covernet_fluxesand the assembly kernel the energy rename touches.test_breeze_coupling.jlgains a regression test for the prerequisite, since nothing in-tree exercised that path. On a domain bottomed at z = 2 km it asserts thatatmosphere_model's cold start agrees with its own reference state, and that the reference is genuinely reduced to the domain bottom rather than left at the datum — the second check matters because two quantities both anchored at the datum would agree with each other too. Reverted to the pre-fix anchor both assertions fail:max|p - pᵣ|is 22020.6 Pa against a 1e-6 tolerance, and the bottom-cell pressure is 100119.5 Pa where the reduced value is under 91192.5.🤖 Generated with Claude Code