Support boolean comparison operations on fields#5309
Merged
Conversation
… 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]>
glwagner
commented
Feb 18, 2026
- 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]>
…ration
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]>
Adding >, <, >=, <= as binary operations on fields introduces 10 new method ambiguities with Base's definitions. Co-Authored-By: Claude Opus 4.6 <[email protected]>
glwagner
commented
Feb 18, 2026
Comment on lines
+64
to
+66
| if T === Union{} || !isconcretetype(T) | ||
| T = eltype(grid) | ||
| end |
Member
Author
There was a problem hiding this comment.
is it also possible to just evaluate the kernel? eg something like
k = @allowscalar kernel_function(1, 1, 1, grid, arguments...)
T = typeof(k)
Collaborator
There was a problem hiding this comment.
since kernel functions are supposed to be relatively cheap (?), I think this would be fine?
…er 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]>
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]>
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]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5309 +/- ##
==========================================
+ Coverage 70.95% 73.25% +2.30%
==========================================
Files 389 397 +8
Lines 22287 22460 +173
==========================================
+ Hits 15813 16453 +640
+ Misses 6474 6007 -467
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:
|
2 tasks
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
3 participants
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.
And infer eltype from AbstractOperations in Field building.
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
cc @mmr0 @navidcy