(0.105.0) Implement interface for stepping closure prognostic variables#5274
(0.105.0) Implement interface for stepping closure prognostic variables#5274
Conversation
src/Models/HydrostaticFreeSurfaceModels/update_hydrostatic_free_surface_model_state.jl
Outdated
Show resolved
Hide resolved
src/Models/NonhydrostaticModels/compute_nonhydrostatic_tendencies.jl
Outdated
Show resolved
Hide resolved
src/Models/HydrostaticFreeSurfaceModels/update_hydrostatic_free_surface_model_state.jl
Outdated
Show resolved
Hide resolved
…mments Rename the `diffusivities` local variable / function argument to `closure_fields` throughout model tendency kernels, update_state helpers, and TKEBasedVerticalDiffusivities internals. Also renames struct types CATKEDiffusivityFields → CATKEClosureFields and TKEDissipationDiffusivityFields → TKEDissipationClosureFields, and kernel functions compute_CATKE_diffusivities! → compute_CATKE_closure_fields! and compute_TKEDissipation_diffusivities! → compute_TKEDissipation_closure_fields!. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…tions Rename the `diffusivities` variable/argument to `closure_fields` across all closure implementation files for consistency with the earlier rename of the container and top-level function. Co-Authored-By: Claude Opus 4.6 <[email protected]>
src/Models/HydrostaticFreeSurfaceModels/update_hydrostatic_free_surface_model_state.jl
Outdated
Show resolved
Hide resolved
...eClosures/turbulence_closure_implementations/TKEBasedVerticalDiffusivities/catke_equation.jl
Outdated
Show resolved
Hide resolved
...nceClosures/turbulence_closure_implementations/convective_adjustment_vertical_diffusivity.jl
Outdated
Show resolved
Hide resolved
src/TurbulenceClosures/turbulence_closure_implementations/scalar_diffusivity.jl
Outdated
Show resolved
Hide resolved
- Add initialize_closure_fields! interface for proper closure initialization - Fix Lagrangian averaging bug: use accumulated time instead of stage Δt - Remove NaN sentinel pattern from DynamicSmagorinsky - Remove obsolete time-based conditionals from CATKE and TKE-dissipation - Add initialize! functions to NonhydrostaticModel and HydrostaticFreeSurfaceModel Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Remove unused commented buoyancy_flux wrapper from catke_equation.jl - Remove commented compute_closure_fields! from scalar_diffusivity.jl - Fix alignment in convective_adjustment_vertical_diffusivity.jl Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Only step DynamicSmagorinsky at stage 1 to avoid recomputing 3x per iteration with RK3 - Add initialize_closure_fields! fallback for nothing closure Co-Authored-By: Claude Opus 4.6 <[email protected]>
When multiple closures are used, iterate through each closure and initialize its corresponding fields. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5274 +/- ##
==========================================
- Coverage 73.51% 73.49% -0.02%
==========================================
Files 395 394 -1
Lines 22194 22234 +40
==========================================
+ Hits 16316 16341 +25
- Misses 5878 5893 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
this is ready to merge |
| implicit_solver = model.timestepper.implicit_solver | ||
| β = model.timestepper.β[model.clock.stage] # Get the correct β value for the current stage | ||
| Δτ = model.clock.last_Δt / β | ||
| Δτ = Δt |
There was a problem hiding this comment.
| Δτ = Δt |
we can use Δt
There was a problem hiding this comment.
wait no, this is wrong. we need to substep CATKE if it has a smaller time-step than Δt
There was a problem hiding this comment.
@simone-silvestri I added substepping here. Please confirm this makes sense.
...rbulence_closure_implementations/TKEBasedVerticalDiffusivities/catke_vertical_diffusivity.jl
Outdated
Show resolved
Hide resolved
...rbulence_closure_implementations/TKEBasedVerticalDiffusivities/catke_vertical_diffusivity.jl
Outdated
Show resolved
Hide resolved
tomchor
left a comment
There was a problem hiding this comment.
You introduced 4 new test files but I don't see them being called anywhere. Am I missing something?
(Cursor/Claude code sometimes creates these test files that aren't called, so maybe that's what happened here?)
Co-authored-by: Tomás Chor <[email protected]>
Co-authored-by: Tomás Chor <[email protected]>
|
What I changed:
Ready to merge. |
Co-Authored-By: Claude Opus 4.6 <[email protected]>
* Support boolean comparison operations on fields and infer eltype from operations Register comparison operators (>, <, >=, <=) as binary operations on fields, and use Base.promote_op to infer the result type of all AbstractOperation constructors instead of hardcoding eltype(grid). Field(op) now allocates data matching the operation's eltype, so boolean operations produce Bool arrays. Closes #5307 Co-Authored-By: Claude Opus 4.6 <[email protected]> * Fix doctest and operator precedence in comparison tests - Suppress Set output in @binary doctest (non-deterministic ordering) - Wrap comparison `isa` tests in parentheses to fix precedence (`u > v isa T` parses as `u > (v isa T)`, not `(u > v) isa T`) Co-Authored-By: Claude Opus 4.6 <[email protected]> * Fall back to eltype(grid) when promote_op fails for KernelFunctionOperation Base.promote_op can return Union{} or non-concrete types (like Any) for complex kernel functions (e.g. ForcingKernelFunction, buoyancy_perturbation). This caused MethodErrors when trying to allocate data arrays. Fall back to eltype(grid) when promote_op cannot infer a concrete return type. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Bump ambiguity threshold from 345 to 355 for comparison operators Adding >, <, >=, <= as binary operations on fields introduces 10 new method ambiguities with Base's definitions. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Refactor operation constructors: accept T as parameter, infer at higher level - All operation inner constructors now accept `::Type{T}` as an optional final argument with a sensible default, making them clean passthroughs - BinaryOperation, UnaryOperation, MultiaryOperation: default T via `Base.promote_op` (reliable for simple eltypes) - KernelFunctionOperation inner constructor takes `(kernel_function, grid, arguments::Tuple, T=eltype(grid))` — no argument splatting, usable as internal interface. Outer convenience constructor splats + infers T. - adapt_structure, on_architecture, and multi-region getregion pass `eltype(op)` explicitly, avoiding redundant re-inference Co-Authored-By: Claude Opus 4.6 <[email protected]> * Remove promote_op from KernelFunctionOperation constructor Kernel functions are too complex for Base.promote_op to reliably infer return types. For example, SeawaterPolynomials.ρ with a Float32 grid but Float64 EOS coefficients causes promote_op to return Float64, changing the KFO eltype from Float32 to Float64 and breaking Float32 seawater density tests. Revert to always using eltype(grid) for KFO, as on main. The promote_op eltype inference is still used for Binary, Unary, and Multiary operations where it works reliably. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Remove promote_op from KFO outer constructor, use eltype(grid) default promote_op inferred Float64 for TEOS10 density on Float32 grids (because EOS polynomial coefficients are Float64), causing precision mismatches in seawater density tests. The inner constructor already defaults to eltype(grid); users can override T explicitly via the inner constructor when needed. Co-Authored-By: Claude Opus 4.6 <[email protected]> * Retrigger CI --------- Co-authored-by: Claude Opus 4.6 <[email protected]>

This PR creates an interface called
which can be extended by closures in order to step forward prognostic fields outside the models default time-stepping mechanism. This is useful for closures that implement off-pattern time-stepping methods, such as Lagrangian advection by DynamicSmagorinsky, or substepping, like CATKEVerticalDiffusivity and TKEDissipationVerticalDiffusivity. CATKEVerticalDiffusivity also evolves a non-advected prognostic variable that represents an Eulerian time-average of the surface buoyancy flux.
The new interface removes state-dependent conditionals in
compute_closure_fields!, which complicate preserving the "idempotency" ofupdate_state!(ie repeated calls toupdate_state!should not change the prognostic state of the model). This makes checkpointing easier, and also simplifies raising via Reactant.I have also taken the opportunity to clean up the semantics of the closure interface.
It also adds
initialize_closure_fields!which is needed to remove a conditional in DynamicSmagorinsky.Takes over from #5267