Close u/v momentum budget on HydrostaticFreeSurfaceModel#233
Open
tomchor wants to merge 3 commits into
Open
Conversation
Add two pieces of machinery to make `UMomentumEquation`/`VMomentumEquation` budgets
close on `HydrostaticFreeSurfaceModel`:
1. `Advection(::HydrostaticFreeSurfaceModel)` now wraps `U_dot_∇u`/`U_dot_∇v` (the
vector-invariant kernel that `hydrostatic_free_surface_*_velocity_tendency`
actually calls) instead of `div_𝐯u`/`div_𝐯v`. The `Advection` type alias
becomes a Union of both kernel parameterisations so existing `isa Advection`
checks keep working for NH. NH `Advection` is unchanged.
2. New `BarotropicPressureGradient` diagnostic on U and V wraps
`explicit_barotropic_pressure_{x,y}_gradient(free_surface)`. For
`ImplicitFreeSurface` and `SplitExplicitFreeSurface` this returns zero
(the barotropic contribution is solved implicitly), and for the NH
`free_surface = nothing` case it also returns zero — but for
`ExplicitFreeSurface` it equals `g ∂_i η` and is the missing term needed
for closure.
Add HFS budget closure tests for U and V (using the default
`ImplicitFreeSurface`) plus matching `BarotropicPressureGradient` exports
and aliases. Update the docs page with a dedicated HFS budget section
that spells out the formula and the free-surface free-surface-type caveat.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends Oceanostics’ momentum-budget diagnostics to fully close the u/v momentum budgets for HydrostaticFreeSurfaceModel (HFS), bringing HFS coverage in line with the existing NonhydrostaticModel budget-closure work from #213.
Changes:
- Update HFS
Advectiondiagnostics to wrap the vector-invariant kernels (U_dot_∇u/U_dot_∇v) used byhydrostatic_free_surface_{u,v}_velocity_tendency. - Add
BarotropicPressureGradient(u/v) diagnostics wrapping Oceananigans’ explicit barotropic free-surface pressure-gradient kernels. - Add HFS u/v budget-closure tests and update the documentation to describe the HFS budget formula and caveats.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/UMomentumEquation.jl |
Adds HFS advection wrapper (U_dot_∇u) and introduces BarotropicPressureGradient for u-momentum. |
src/VMomentumEquation.jl |
Adds HFS advection wrapper (U_dot_∇v) and introduces BarotropicPressureGradient for v-momentum. |
src/Oceanostics.jl |
Re-exports the new prefixed barotropic-pressure-gradient diagnostics. |
test/test_u_momentum_diagnostics.jl |
Adds u-momentum HFS budget-closure test and runs it in the main testset. |
test/test_v_momentum_diagnostics.jl |
Adds v-momentum HFS budget-closure test and runs it in the main testset. |
docs/src/momentum_equation.md |
Documents the HFS u/v budget and adds the new diagnostics to the rendered API docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…files Agent-Logs-Url: https://github.com/tomchor/Oceanostics.jl/sessions/c68afc04-9053-4b56-8839-a23f07f8585f Co-authored-by: tomchor <13205162+tomchor@users.noreply.github.com>
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
The earlier HFS Advection refactor only added a 3-arg HFS-typed method (model, scheme, velocities) and an HFS-typed 1-arg convenience method. The generic 5-arg overload `Advection(model, u, v, w, scheme)` was left untyped on `model`, so an explicit call `Advection(hfs_model, u, v, w, scheme)` dispatched to the NH `div_𝐯u`/`div_𝐯v` form — inconsistent with the documented behaviour and with the 1-arg form's `U_dot_∇u`/`U_dot_∇v`. Add an HFS-typed 5-arg overload that builds `velocities = (; u, v, w)` and forwards to the HFS 3-arg path. With both construction routes now consistent, the `momentum_advection = Centered()` workaround required to make the test_*_momentum_terms HFS branches pass is no longer necessary — drop it everywhere so the tests exercise HFS's default VectorInvariant scheme, and update the docs caveat to reflect that both Advection construction paths use the vector-invariant kernel. Addresses Copilot review comments on PR #233: - src/UMomentumEquation.jl:105 (Copilot) - src/VMomentumEquation.jl:104 (Copilot) - docs/src/momentum_equation.md:126 (Copilot) - test/test_u_momentum_diagnostics.jl:318 (Copilot) - test/test_v_momentum_diagnostics.jl:318 (Copilot) Co-Authored-By: Claude Opus 4.7 (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.
Summary
Closes the u/v momentum budget on
HydrostaticFreeSurfaceModel. Follow-up to #213, which scoped the budget closure tests toNonhydrostaticModelonly because two pieces were missing. This PR adds them.Changes
Advection(::HydrostaticFreeSurfaceModel)now wrapsU_dot_∇u/U_dot_∇v(the vector-invariant kernel thathydrostatic_free_surface_*_velocity_tendencyactually calls), instead ofdiv_𝐯u/div_𝐯v. TheAdvectiontype alias becomes a Union of both kernel parameterisations:isa UAdvectionkeeps working for both NH and HFS. NH behaviour is unchanged.New
BarotropicPressureGradientdiagnostic on U and V, wrappingexplicit_barotropic_pressure_{x,y}_gradient(free_surface). Behaviour by free-surface type:nothing(NH)zero(grid)ImplicitFreeSurface(HFS default)zero(grid)— barotropic part solved implicitlySplitExplicitFreeSurfacezero(grid)— handled inside the split-explicit solverExplicitFreeSurfaceg ∂_i ηHFS u/v budget
No
BuoyancyAcceleration(absorbed intopHY′), no Stokes terms (HFS has nostokes_driftfield).Tests / CI
test_u_momentum_hfs_budget_closureandtest_v_momentum_hfs_budget_closurebuild an HFS model with every term active (momentum_advection = Centered()so the advection diagnostic matches the tendency for the test scheme), set non-trivialu,v,bfields, callupdate_state!, and assertinterior(budget) ≈ interior(Tendency)to machine precision. The closure holds onRectilinearGridwithImmersedBoundaryGridsemantics already exercised by the term-construction tests.Existing NH budget closure and W tests are unchanged.
Test plan
TEST_GROUP=u_momentum_diagnostics julia --project -e 'using Pkg; Pkg.test()'TEST_GROUP=v_momentum_diagnostics julia --project -e 'using Pkg; Pkg.test()'TEST_GROUP=w_momentum_diagnostics julia --project -e 'using Pkg; Pkg.test()'build_docsCI)Follow-ups not in this PR
ExplicitFreeSurface—BarotropicPressureGradientreturns the correct kernel for it, but the test would need to be parameterised on free-surface type. The current test only exercises the implicit/zero branch.U_dot_∇u_hydrostatic_metric) is zero onRectilinearGrid, so it's not exposed as a diagnostic; would need a wrapper forLatitudeLongitudeGrid/OrthogonalSphericalShellGridusers.