Skip to content

materialize DefaultBoundaryCondition from bcs in Field constructor #4762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

materialize DefaultBoundaryCondition from bcs in Field constructor #4762

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d64fab4
change this
simone-silvestri Sep 3, 2025
4ba486f
try like this
simone-silvestri Sep 3, 2025
1cafd7a
regularize
simone-silvestri Sep 4, 2025
4688738
Merge remote-tracking branch 'origin/main' into ss/build-fields-with-…
simone-silvestri Sep 4, 2025
648e287
regularize
simone-silvestri Sep 4, 2025
9cd7fc3
regularize also multiregion
simone-silvestri Sep 4, 2025
fa689f3
add fallbacks
simone-silvestri Sep 4, 2025
af6ae1a
materialize
simone-silvestri Sep 4, 2025
cc033aa
Merge branch 'main' into ss/build-fields-with-default
simone-silvestri Sep 4, 2025
b0255f8
clean up a bit
simone-silvestri Sep 4, 2025
14b3814
Merge branch 'ss/build-fields-with-default' of github.com:CliMA/Ocean…
simone-silvestri Sep 4, 2025
c997c93
materialize do not regularize
simone-silvestri Sep 4, 2025
d96a8f0
Update regularize_immersed_boundary_condition logic
simone-silvestri Sep 4, 2025
c1874e0
send it
simone-silvestri Sep 4, 2025
98ce374
Fix case sensitivity for missing boundary condition
simone-silvestri Sep 4, 2025
66bf31b
default
simone-silvestri Oct 6, 2025
8a992fc
Merge branch 'main' into ss/build-fields-with-default
simone-silvestri Oct 6, 2025
39dc07f
bugfix
simone-silvestri Oct 6, 2025
2002020
default_prognostic_bc
simone-silvestri Oct 6, 2025
de2bc90
add bc
simone-silvestri Oct 6, 2025
2684601
this should work
simone-silvestri Oct 6, 2025
bc2755a
ok this should cover all the cases
simone-silvestri Oct 6, 2025
d411608
bugfix
simone-silvestri Oct 6, 2025
b61194c
Merge branch 'main' into ss/build-fields-with-default
simone-silvestri Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 47 additions & 23 deletions src/BoundaryConditions/field_boundary_conditions.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,14 @@ using GPUArraysCore
##### Default boundary conditions
#####

struct DefaultBoundaryCondition{BC}
boundary_condition :: BC
end

DefaultBoundaryCondition() = DefaultBoundaryCondition(NoFluxBoundaryCondition())
struct DefaultBoundaryCondition end
Copy link
Member

@glwagner glwagner Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you change this design?

Copy link
Collaborator Author

@simone-silvestri simone-silvestri Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of this PR, I have removed the boundary_condition inside the DefaultBoundaryCondition.
In my opinion, the default is uniquely defined and should depend on location and grid; user-defined BCs should be explicitly specified rather than changing a default globally (which is not even possible because Face and Center locations do not accept the same bcs so this approach could very well lead to error)

Copy link
Collaborator Author

@simone-silvestri simone-silvestri Sep 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought to remove it for this reason and in particular because validate_boundary_conditions is only grid and location dependent (not model dependent), so it basically forces a limited default choice. But of course, I am open to discuss about it and keep this feature if this is the consensus 😅

Copy link
Member

@glwagner glwagner Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, what do you think about models / situations in which "no flux" is irrelevant. For example a model that has no derivatives.

Copy link
Collaborator Author

@simone-silvestri simone-silvestri Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have the default be a nothing like on Faces, this would then be general.

Copy link
Member

@glwagner glwagner Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That isn't general because models with gradients require NoFluxBoundaryCondition() as a default.


default_prognostic_bc(::Grids.Periodic, loc, default) = PeriodicBoundaryCondition()
default_prognostic_bc(::FullyConnected, loc, default) = MultiRegionCommunicationBoundaryCondition()
default_prognostic_bc(::Flat, loc, default) = nothing
default_prognostic_bc(::Bounded, ::Center, default) = default.boundary_condition
default_prognostic_bc(::LeftConnected, ::Center, default) = default.boundary_condition
default_prognostic_bc(::RightConnected, ::Center, default) = default.boundary_condition
default_prognostic_bc(::Bounded, ::Center, default) = NoFluxBoundaryCondition()
default_prognostic_bc(::LeftConnected, ::Center, default) = NoFluxBoundaryCondition()
default_prognostic_bc(::RightConnected, ::Center, default) = NoFluxBoundaryCondition()

# TODO: make model constructors enforce impenetrability on velocity components to simplify this code
default_prognostic_bc(::Bounded, ::Face, default) = ImpenetrableBoundaryCondition()
Expand All @@ -36,12 +32,13 @@ _default_auxiliary_bc(::Bounded, ::Face) = nothing
_default_auxiliary_bc(::RightConnected, ::Face) = nothing
_default_auxiliary_bc(::LeftConnected, ::Face) = nothing

default_auxiliary_bc(grid, ::Val{:east}, loc) = _default_auxiliary_bc(topology(grid, 1)(), loc[1])
default_auxiliary_bc(grid, ::Val{:west}, loc) = _default_auxiliary_bc(topology(grid, 1)(), loc[1])
default_auxiliary_bc(grid, ::Val{:south}, loc) = _default_auxiliary_bc(topology(grid, 2)(), loc[2])
default_auxiliary_bc(grid, ::Val{:north}, loc) = _default_auxiliary_bc(topology(grid, 2)(), loc[2])
default_auxiliary_bc(grid, ::Val{:bottom}, loc) = _default_auxiliary_bc(topology(grid, 3)(), loc[3])
default_auxiliary_bc(grid, ::Val{:top}, loc) = _default_auxiliary_bc(topology(grid, 3)(), loc[3])
default_auxiliary_bc(grid, ::Val{:east}, loc) = _default_auxiliary_bc(topology(grid, 1)(), loc[1])
default_auxiliary_bc(grid, ::Val{:west}, loc) = _default_auxiliary_bc(topology(grid, 1)(), loc[1])
default_auxiliary_bc(grid, ::Val{:south}, loc) = _default_auxiliary_bc(topology(grid, 2)(), loc[2])
default_auxiliary_bc(grid, ::Val{:north}, loc) = _default_auxiliary_bc(topology(grid, 2)(), loc[2])
default_auxiliary_bc(grid, ::Val{:bottom}, loc) = _default_auxiliary_bc(topology(grid, 3)(), loc[3])
default_auxiliary_bc(grid, ::Val{:top}, loc) = _default_auxiliary_bc(topology(grid, 3)(), loc[3])
default_auxiliary_bc(grid, ::Val{:immersed}, loc) = nothing

#####
##### Field boundary conditions
Expand Down Expand Up @@ -127,14 +124,13 @@ and the topology in the boundary-normal direction is used:
- `ImpenetrableBoundaryCondition` for `Bounded` directions and `Face`-located fields
- `nothing` for `Flat` directions and/or `Nothing`-located fields
"""
FieldBoundaryConditions(default_bounded_bc::BoundaryCondition = NoFluxBoundaryCondition();
west = DefaultBoundaryCondition(default_bounded_bc),
east = DefaultBoundaryCondition(default_bounded_bc),
south = DefaultBoundaryCondition(default_bounded_bc),
north = DefaultBoundaryCondition(default_bounded_bc),
bottom = DefaultBoundaryCondition(default_bounded_bc),
top = DefaultBoundaryCondition(default_bounded_bc),
immersed = DefaultBoundaryCondition(default_bounded_bc)) =
FieldBoundaryConditions(; west = DefaultBoundaryCondition(),
east = DefaultBoundaryCondition(),
south = DefaultBoundaryCondition(),
north = DefaultBoundaryCondition(),
bottom = DefaultBoundaryCondition(),
top = DefaultBoundaryCondition(),
immersed = DefaultBoundaryCondition()) =
Copy link
Member

@glwagner glwagner Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this PR removes a nice feature that I designed for changing the default boundary condition. Note that this code is meant to be applicable to a wide class of models --- not just NonhydrostaticModel and HydrostaticFreeSurfaceModel.

Copy link
Member

@glwagner glwagner Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this PR makes the previous code more useful...

Copy link
Collaborator Author

@simone-silvestri simone-silvestri Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can keep the default in the DefaultBoundaryCondition. I guess having it or not is not really the scope of this PR

FieldBoundaryConditions(west, east, south, north, bottom, top, immersed)

"""
Expand Down Expand Up @@ -181,9 +177,37 @@ function FieldBoundaryConditions(grid::AbstractGrid, loc, indices=(:, :, :);
immersed = DefaultBoundaryCondition())

bcs = FieldBoundaryConditions(indices, west, east, south, north, bottom, top, immersed)
return regularize_field_boundary_conditions(bcs, grid, loc)
return materialize_default_boundary_conditions(bcs, grid, loc)
end

#####
##### Boundary condition "materialization" (materializes a `DefaultBoundaryCondition` that is grid and location dependent)
#####

function materialize_default_boundary_conditions(bcs::FieldBoundaryConditions,
grid::AbstractGrid,
loc::Tuple)

west = materialize_default_boundary_condition(bcs.west, Val(:west), grid, loc)
east = materialize_default_boundary_condition(bcs.east, Val(:east), grid, loc)
south = materialize_default_boundary_condition(bcs.south, Val(:south), grid, loc)
north = materialize_default_boundary_condition(bcs.north, Val(:north), grid, loc)
bottom = materialize_default_boundary_condition(bcs.bottom, Val(:bottom), grid, loc)
top = materialize_default_boundary_condition(bcs.top, Val(:top), grid, loc)

immersed = materialize_default_boundary_condition(bcs.immersed, Val(:immersed), grid, loc)

return FieldBoundaryConditions(west, east, south, north, bottom, top, immersed)
end

# nothing and missing remain nothing and missing
materialize_default_boundary_conditions(::Nothing, args...) = nothing
materialize_default_boundary_conditions(::Missing, args...) = missing

# Individual materialize
materialize_default_boundary_condition(bc, args...) = bc # fallback
materialize_default_boundary_condition(::DefaultBoundaryCondition, side, grid, loc) = default_auxiliary_bc(grid, side, loc)

#####
##### Boundary condition "regularization"
#####
Expand Down
2 changes: 1 addition & 1 deletion src/BoundaryConditions/show_boundary_conditions.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ bc_str(zbc::ZBC) = "Zipper($(zbc.condition))"
##### BoundaryCondition
#####

Base.summary(bc::DFBC) = string("DefaultBoundaryCondition (", summary(bc.boundary_condition), ")")
Base.summary(bc::DFBC) = string("DefaultBoundaryCondition")
Base.summary(bc::OBC{Open{MS}}) where MS = string("OpenBoundaryCondition{$MS}: ", prettysummary(bc.condition))
Base.summary(bc::IBC) = string("ImpenetrableBoundaryCondition")
Base.summary(bc::FBC) = string("FluxBoundaryCondition: ", prettysummary(bc.condition))
Expand Down
12 changes: 8 additions & 4 deletions src/Fields/field.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Oceananigans.BoundaryConditions: OBC, MCBC, BoundaryCondition, Zipper, construct_boundary_conditions_kernels
using Oceananigans.BoundaryConditions: OBC, MCBC, Zipper, construct_boundary_conditions_kernels
using Oceananigans.BoundaryConditions: BoundaryCondition, materialize_default_boundary_conditions
using Oceananigans.Grids: parent_index_range, index_range_offset, default_indices, all_indices, validate_indices
using Oceananigans.Grids: index_range_contains
using Oceananigans.Architectures: convert_to_device
Expand Down Expand Up @@ -99,9 +100,12 @@ validate_boundary_condition_location(bc::Zipper, loc::Face, side) =

# Common outer constructor for all field flavors that performs input validation
function Field(loc::Tuple{<:LX, <:LY, <:LZ}, grid::AbstractGrid, data, bcs, indices, op=nothing, status=nothing) where {LX, LY, LZ}
@apply_regionally indices = validate_indices(indices, loc, grid)
@apply_regionally validate_field_data(loc, data, grid, indices)
@apply_regionally validate_boundary_conditions(loc, grid, bcs)
@apply_regionally begin
bcs = materialize_default_boundary_conditions(bcs, grid, loc)
indices = validate_indices(indices, loc, grid)
validate_field_data(loc, data, grid, indices)
validate_boundary_conditions(loc, grid, bcs)
end
buffers = communication_buffers(grid, data, bcs)
return Field{LX, LY, LZ}(grid, data, bcs, indices, op, status, buffers)
end
Expand Down
10 changes: 7 additions & 3 deletions src/ImmersedBoundaries/immersed_boundary_condition.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
using Oceananigans.BoundaryConditions: BoundaryCondition,
DefaultBoundaryCondition,
NoFluxBoundaryCondition,
LeftBoundary,
RightBoundary,
regularize_boundary_condition,
VBC, GBC, FBC, Flux

import Oceananigans.BoundaryConditions: regularize_immersed_boundary_condition,
bc_str,
update_boundary_condition!
update_boundary_condition!,
default_auxiliary_bc,
bc_str

struct ImmersedBoundaryCondition{W, E, S, N, B, T}
west :: W
Expand Down Expand Up @@ -65,7 +67,7 @@ const ZFBC = BoundaryCondition{Flux, Nothing}
regularize_immersed_boundary_condition(ibc::ZFBC, ibg::IBG, args...) = ibc # keep it

regularize_immersed_boundary_condition(default::DefaultBoundaryCondition, ibg::IBG, loc, field_name, args...) =
regularize_immersed_boundary_condition(default.boundary_condition, ibg, loc, field_name, args...)
regularize_immersed_boundary_condition(NoFluxBoundaryCondition(), ibg, loc, field_name, args...)

# Convert certain non-immersed boundary conditions to immersed boundary conditions
function regularize_immersed_boundary_condition(ibc::Union{VBC, GBC, FBC}, ibg::IBG, loc, field_name, args...)
Expand Down Expand Up @@ -97,6 +99,8 @@ Adapt.adapt_structure(to, bc::ImmersedBoundaryCondition) = ImmersedBoundaryCondi

update_boundary_condition!(bc::ImmersedBoundaryCondition, args...) = nothing

default_auxiliary_bc(::IBG, ::Val{:immersed}, loc) = NoFluxBoundaryCondition()

#####
##### Alternative implementation for immersed flux divergence
#####
Expand Down