Introduce AbstractLocation hierarchy and replace Nothing with Reduced#5614
Introduce AbstractLocation hierarchy and replace Nothing with Reduced#5614glwagner wants to merge 15 commits into
Conversation
…ced`
Adds `abstract type AbstractLocation` with `Center`, `Face`, and a new
`Reduced` singleton, and constrains `AbstractField{LX,LY,LZ,...}` so
location parameters must subtype `AbstractLocation`. Reduced and Flat
dimensions, which previously used `Nothing` as a location, now use
`Reduced`.
This eliminates the type piracy flagged in #5601: every `AbstractOperations`
operator signature now dispatches on Oceananigans-owned types
(`LocationTuple`, `LocationTypeTuple`) instead of bare `Base.Tuple`s.
After this change `*((Nothing, Nothing, Nothing), nothing, nothing)`
correctly throws `MethodError` even after `using Oceananigans` — see the
regression test added to `test_quality_assurance.jl`.
Naming alternatives we considered for the new type: `Reduced` (chosen,
matches existing `ReducedField`/`reduced_dimensions` vocabulary),
`Collapsed`, `Singleton`, and `Nil`. `Reduced` won on cohesion with the
codebase even though it names the *state of the dimension* rather than
a position on the cell.
This is a breaking change for downstream code that writes
`Field{Nothing, ...}` type literals, so the version is bumped to 0.109.0.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b60cc80 to
1ca7f8f
Compare
| const C = Center | ||
|
|
||
| const LocationTuple = Tuple{<:AbstractLocation, <:AbstractLocation, <:AbstractLocation} | ||
| const LocationTypeTuple = Tuple{Type{<:AbstractLocation}, Type{<:AbstractLocation}, Type{<:AbstractLocation}} |
There was a problem hiding this comment.
I see LLMs don't understand Julia's type system. This tuple doesn't make sense.
julia> typeof((Reduced, Reduced, Reduced))
Tuple{DataType, DataType, DataType}there's no way that's matched by LocationTypeTuple
There was a problem hiding this comment.
ahah no that is me not understanding it
There was a problem hiding this comment.
ok no I asked for LocationTuple but not the "type tuple". ummm
There was a problem hiding this comment.
ahah no that is me not understanding it
I tried hard to make ChatGPT reason about it, and it always gave me falsities like the following will be true
julia> (Reduced,) isa Tuple{Type{Reduced}}
falsedo we need
<:Type{<:AbstractLocation}is that right?
No, any of this can't possibly work, because the type in the type parameter will always have type DataType, so you can't really dispatch on something related to AbstractLocation.
There was a problem hiding this comment.
LLMs don't understand anything at all btw -- not just Julia's type system. "Understanding" is not the point of an LLM
|
|
||
| # instantiate location if types are passed | ||
| $op(Lc::Tuple, a, b) = $op((Lc[1](), Lc[2](), Lc[3]()), a, b) | ||
| $op(Lc::$LocationTypeTuple, a, b) = $op((Lc[1](), Lc[2](), Lc[3]()), a, b) |
There was a problem hiding this comment.
Replacing Nothing with Reduced is definitely a move in the right direction, but this tuple-based method (together with all the similar ones below) is still problematic: as mentioned above, using a tuple here is flawed (and LocationTypeTuple in particular is useless), because tuples are special
There was a problem hiding this comment.
we can instantiate the locations upon input, before they arrive in this function
| const C = Center | ||
|
|
||
| const LocationTuple = Tuple{<:AbstractLocation, <:AbstractLocation, <:AbstractLocation} | ||
| const LocationTypeTuple = Tuple # {Type{<:AbstractLocation}, Type{<:AbstractLocation}, Type{<:AbstractLocation}} |
There was a problem hiding this comment.
LocationTypeTuple = Tuple is not right because this is still type piracy (the test checking for type piracies in AbstractOperators will fail), but I want to see what else breaks besides the type piracy
There was a problem hiding this comment.
can I remove LocationTypeTuple or are you still testing?
There was a problem hiding this comment.
The non-distributed tests on Buildkite haven't started yet, I was hoping to see what still fails.
Of the tests in GitHub Actions, most of them are now passing, but I'm confused by https://github.com/CliMA/Oceananigans.jl/actions/runs/26230990735/job/77191416849?pr=5614#step:6:570
Distributed output combining - RectilinearGrid (2x2): Error During Test at /home/runner/work/Oceananigans.jl/Oceananigans.jl/test/test_distributed_output_combining.jl:331
Got exception outside of a @test
BoundsError: attempt to access 10×10×1 Array{Float32, 3} at index [4:7, 4:7, 4:7]
Stacktrace:
[1] throw_boundserror(A::Array{Float32, 3}, I::Tuple{UnitRange{Int64}, UnitRange{Int64}, UnitRange{Int64}})
@ Base ./essentials.jl:15
[2] checkbounds
@ ./abstractarray.jl:699 [inlined]
[3] view(::Array{Float32, 3}, ::UnitRange{Int64}, ::UnitRange{Int64}, ::UnitRange{Int64})
@ Base ./subarray.jl:214
[4] load_combined_field_data!(field::Field{Center, Center, Reduced, Nothing, RectilinearGrid{Float64, Periodic, Periodic, Bounded, Oceananigans.Grids.StaticVerticalDiscretization{OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, Float64, Float64}, Float64, Float64, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, CPU}, Tuple{Colon, Colon, Colon}, OffsetArray{Float64, 3, SubArray{Float64, 3, Array{Float64, 4}, Tuple{Base.Slice{Base.OneTo{Int64}}, Base.Slice{Base.OneTo{Int64}}, Base.Slice{Base.OneTo{Int64}}, Int64}, true}}, Float64, Nothing, Oceananigans.Fields.FixedTime{Float64}, Nothing}, all_ranks::Vector{Oceananigans.OutputReaders.RankOutputData{RectilinearGrid{Float64, FullyConnected, FullyConnected, Bounded, Oceananigans.Grids.StaticVerticalDiscretization{OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, Float64, Float64}, Float64, Float64, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, OffsetVector{Float64, StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}}, Distributed{CPU, false, Partition{Int64, Int64, Nothing}, Tuple{Int64, Int64, Int64}, Int64, Tuple{Int64, Int64, Int64}, Oceananigans.DistributedComputations.NeighboringRanks{Int64, Int64, Int64, Int64, Int64, Int64, Int64, Int64}, MPI.Comm, Vector{MPI.Request}, Base.RefValue{Int64}, Nothing}}, Tuple{Int64, Int64, Int64}}}, name::String, iter::Int64; reader_kw::@NamedTuple{})
@ Oceananigans.OutputReaders ~/work/Oceananigans.jl/Oceananigans.jl/src/OutputReaders/combining_field_time_series.jl:452
[...]
There was a problem hiding this comment.
let me know when you're ready for more commits here
There was a problem hiding this comment.
Feel free to push now, thanks for waiting
There was a problem hiding this comment.
I think I can fix this as well. Since we require the types (not Type{location}) we have to instantiate at all entry points. Doing so in broadcasting fixes this error
|
@navidcy @simone-silvestri this PR is going to cause a nuclear explosion throughout the ecosystem. We need to be absolutely sure that "Reduced" is the name we want here. There is some illogic in it, because it is not actually a location at all. Instead it refers to (supposed) the operation that produced the data. "Nothing" was actually a good name, because it is more explicitly true. Another one could be "All" ... I think it's worth discussing before merging this PR hastily |
|
What about |
Those do seem better. One crucial property of this location is that it implies that the "nothing" location is extruded for operations. In other words, if we form This concept could be captured by Claude also suggested |
`LocationTypeTuple = Tuple{Type{<:AbstractLocation}, ...}` never actually
dispatched: `typeof((Center, Reduced, Reduced)) === Tuple{DataType, DataType, DataType}`
which is not a subtype of the parametric form. The `$op(Lc::LocationTypeTuple, ...)`
fallbacks in `unary_operations.jl`, `binary_operations.jl`, and
`multiary_operations.jl` were therefore dead code — and with the temporary
`LocationTypeTuple = Tuple` they were still pirate.
Per @glwagner: instantiate the locations upon input, before they arrive in
the operator function. Concretely:
- Delete the `$op(Lc::LocationTypeTuple, ...)` fallback methods in
unary/binary/multiary operations and the parallel
`grid_metric_operation(Loc::Tuple, ...)` in `grid_metrics.jl`.
- Switch the internal callers in `boundary_transport.jl` and
`boundary_mean.jl` to pass instantiated tuples
(`(Face(), Center(), Center())` etc.).
- Update the `grid_metrics.jl` doctest and `test_abstract_operations.jl`
test cases to use instance-form tuples.
- Remove the `LocationTypeTuple` const and export entirely from
`src/Grids/Grids.jl` and the corresponding import in
`src/AbstractOperations/AbstractOperations.jl`.
`*((Center(), Center(), Center()), Δx, c)` still works.
`*((Center, Center, Center), Δx, c)` now throws `MethodError` — type-form
sugar for the operator API is gone (breaking change, documented).
`Aqua.test_piracies(Oceananigans.AbstractOperations)` passes.
Also fix the distributed-output combining failure reported in PR #5614:
`flatten_nothing_dimension(::Nothing, range) = 1:1` in
`combining_field_time_series.jl` only matched `nothing`, so reduced fields
(now carrying `Reduced()` instances) fell through to the generic method
that returned the full range, triggering a `BoundsError` when loading data
into a 1-element z slice. Renamed to `flatten_reduced_dimension` and
retyped on `::Reduced`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`Base.copyto!(::Field, ::Broadcasted)` was passing `location(dest)` — a tuple of singleton location *types* like `(Center, Center, Center)` — into `broadcasted_to_abstract_operation`, which then forwarded it as the first argument to `bc.f`. For broadcasts like `pNHS ./= Δt`, that produced `/((Center, Center, Center), pNHS, Δt)`, which relied on the type-tuple operator fallback we just removed. Switch to `instantiated_location(dest)` so the operator only ever sees instance-form tuples (`LocationTuple`), matching the convention used throughout the operator codegen. This fixes the MPI `test_distributed_output_combining.jl` failure surfaced in PR #5614: MethodError: no method matching /(::Tuple{DataType, DataType, DataType}, ::Field{Center, Center, Center, ...}, ::Float64) Verified locally with `JULIA_PROJECT=test julia --project=test test/test_distributed_output_combining.jl`: - RectilinearGrid (2x2): 18/18 - RectilinearGrid slab (1x4): 6/6 - LatitudeLongitudeGrid (1x4): pass - TripolarGrid (1x4): 6/6 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`pNHS ./= Δt⁺` went through the Field broadcast pipeline, which builds an AbstractOperation, infers a location tuple, and launches a kernel — heavy machinery for a scalar in-place divide where every cell (interior + halo) gets the same treatment. Switch to `parent(pNHS) ./= Δt⁺` so the broadcast goes straight to the OffsetArray storage, matching the idiom used in `distributed_fields.jl`, `conjugate_gradient_solver.jl`, and elsewhere. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
More thoughts:
|
|
Math / set-theory family
Geometric family
English "just one" family
Greek/Latin roots
Programming / API ecosystem
|
|
|
|
Maybe I'm missing the point, but what about |
acronym ❌ more seriously, I don't think it is interpretable, But |
|
Interesting that both
|
Indeed, this is a philosophical question. Does the field have "all" locations (this is how it is treated in broadcasting) or does it have "no location" (which implies that it must be broadcasted with all locations of a 3D field) |
|
I'm kinda late to the discussion so forgive me if I'm missing something, but does it make sense to lump together the location for a To me it makes sense that a |
|
@tomchor I'm not entirely sure how to achieve your suggestion. For example what should We have strived to always return the type asked for by the user (eg if you read a type from a constructor, it unambiguously tells you the type of the object that is constructed). I don't think we can preserve this convention without taking the erroring approach. But on the other hand, I worry that throwing such an error is inconvenient. It might require a lot of special casing, especially in downstream packages. This is actually the benefit of distinguishing between the grid topology and the field location, I think. It allows us to write more general code more easily. |
Hmm, I see your point. Thanks for clarifying! |
In part this lesson was learned when we switched the |
Summary
Introduces
abstract type AbstractLocationwithCenter,Face, and a new singletonReduced, and constrains the type parameters ofAbstractField{LX, LY, LZ, ...}so that location parameters must subtypeAbstractLocation. Reduced andFlatdimensions, which previously usedNothingas a location, now useReduced.Fixes the
AbstractOperationstype piracy flagged in #5601:AbstractOperations(+,-,*,/,^,<,>,<=,>=,atan,atand,mod, plus the unary forms) now dispatches on Oceananigans-owned types (LocationTuple,LocationTypeTuple) instead of bareBase.Tuples.*((Nothing, Nothing, Nothing), nothing, nothing)correctly throwsMethodErroreven afterusing Oceananigans— see the regression added totest_quality_assurance.jl.AbstractOperationsis removed from thepirate_modules = (...)skip list in the Aqua test, andAqua.test_piracies(Oceananigans.AbstractOperations)now passes.Naming
We picked
Reducedbecause it cohabits cleanly with existing vocabulary in the codebase:ReducedField,XReducedField,reduced_dimensions(::AbstractField), etc. Alternatives we considered:Reduced(chosen) — matches existing aliases, no rename cascade, semantically clear in the reduction context. Honest critique: names the state of the dimension (past-tense) rather than a position on the cell the wayCenterandFacedo. It also has to cover both "a dim that got averaged/integrated out" and "aFlat-topology dim that was never there." Semantically these are one concept (no spatial coordinate in this slot), but the name leans toward the reduction story.Collapsed— similarly state-describing, and less tightly tied to existing terms.Singleton— describes the size-1 array dimension neutrally on cause. Less tied to existing API.Nil— short and "nothing-like", but cryptic.Happy to revisit if any of those land better with reviewers.
Breaking
This breaks any downstream code that writes
Field{Nothing, ...}(orAbstractField{Nothing, ...}, or matches(Nothing, Center, Center)location tuples in dispatch). All such sites insrc/,test/, andext/are updated. The Julia pre-1.0 convention puts breaking changes in the minor version, so this bumps0.108.0 → 0.109.0.What changed
src/Grids/Grids.jl— newAbstractLocation,Reduced,LocationTuple,LocationTypeTuple;CenterandFaceretroactively made subtypes; types exported fromOceananigans.src/Fields/abstract_field.jl—AbstractField{LX, LY, LZ, G, T, N}now requiresLX/LY/LZ <: AbstractLocation.src/Fields/field.jl,src/Fields/abstract_field.jl— theXReducedField,XReducedAF,ReducedFieldand related aliases now useReducedinstead ofNothing.src/Fields/show_fields.jl—location_str(::Type{Reduced}) = "⋅".src/AbstractOperations/{unary,binary,multiary}_operations.jl— operator codegen retyped;choose_location(::Reduced, ...)etc.AbstractOperations.Location = AbstractLocation.src/Grids/{grid_utils,nodes_and_spacings,vertical_discretization,new_data,input_validation}.jl— all::Nothingdispatches that take a location switched to::Reduced.src/Fields/{scans,interpolate}.jl— same;reduced_locationnow returnsReduced/Reduced();filter_nothing_dimsrenamed tofilter_reduced_dimswith predicateloc[d] isa Reduced.src/BoundaryConditions/{field_boundary_conditions,fill_halo_kernels}.jl—default_prognostic_bc(...)etc. retyped; halo bookkeeping usesReduced.src/Models/{boundary_mean,boundary_condition_operation,NonhydrostaticModels/nonhydrostatic_model,HydrostaticFreeSurfaceModels/SplitExplicitFreeSurfaces/split_explicit_free_surface}.jl,src/ImmersedBoundaries/{grid_fitted_bottom,partial_cell_bottom,active_cells_map,immersed_reductions,mask_immersed_field}.jl,src/MultiRegion/cubed_sphere_grid.jl,src/StokesDrifts.jl,src/DistributedComputations/distributed_immersed_boundaries.jl,src/OutputReaders/field_time_series_indexing.jl,src/TurbulenceClosures/.../catke_vertical_diffusivity.jl—Field{..., Nothing}/(..., Nothing)literals replaced withReduced(the Nothing-as-grid slot is preserved everywhere).ext/OceananigansReactantExt/Grids/Grids.jl—reactant_offset_indices(::Reduced, ...).ext/OceananigansNCDatasetsExt/utils.jl— docstring example updated.test/— allField{Nothing, ...}/FieldTimeSeries{Nothing, ...}/(Nothing, ..., ...)test fixtures updated toReduced.test/test_quality_assurance.jl—AbstractOperationsremoved from the broken-piracy skip list; new regression@test_throws MethodError *((Nothing, Nothing, Nothing), nothing, nothing).Project.toml—version = "0.109.0".Test plan
Oceananigans.AbstractOperationsField{Nothing, ...}literals that need updating downstreamusing Oceananigans; Average/Integral; Field{Center, Center, Reduced}; arithmetic on reduced fields— all green🤖 Generated with Claude Code