Skip to content

Use the correct velocities in VarianceDissipationComputations#5298

Merged
simone-silvestri merged 1 commit intomainfrom
ss/fix-variance-dissipation
Feb 14, 2026
Merged

Use the correct velocities in VarianceDissipationComputations#5298
simone-silvestri merged 1 commit intomainfrom
ss/fix-variance-dissipation

Conversation

@simone-silvestri
Copy link
Copy Markdown
Collaborator

The HydrostaticFreeSurfaceModel has switched to using model.transport_velocities to advect the tracers.
This needs to be reflected also in the VarianceDissipationComputation

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.06%. Comparing base (5842259) to head (0f83287).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5298      +/-   ##
==========================================
+ Coverage   68.79%   73.06%   +4.26%     
==========================================
  Files         395      395              
  Lines       21465    22361     +896     
==========================================
+ Hits        14767    16337    +1570     
+ Misses       6698     6024     -674     
Flag Coverage Δ
buildkite 68.79% <100.00%> (ø)
julia 68.79% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@glwagner
Copy link
Copy Markdown
Member

Is this also a concern for other diagnostics?

@glwagner
Copy link
Copy Markdown
Member

For example, work to close budgets (very popular) needs to know about this.

@navidcy navidcy added the numerics 🧮 So things don't blow up and boil the lobsters alive label Feb 13, 2026
@simone-silvestri
Copy link
Copy Markdown
Collaborator Author

yeah, I have in the plans to enhance the docs with some timestepping information.

@simone-silvestri simone-silvestri merged commit 39a5750 into main Feb 14, 2026
80 checks passed
@simone-silvestri simone-silvestri deleted the ss/fix-variance-dissipation branch February 14, 2026 12:58
glwagner referenced this pull request Feb 20, 2026
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

numerics 🧮 So things don't blow up and boil the lobsters alive

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants