Skip to content

Fix getbc not passing extra args for discrete BC functions #4816

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 7 commits into
base: main
Choose a base branch
from
Open

Fix getbc not passing extra args for discrete BC functions #4816

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fcdecfb
Fix getbc not passing args for discrete BC functions
bgroenks96 Sep 26, 2025
9bbebb8
Improve docstring for DiscreteBoundaryFunction
bgroenks96 Sep 26, 2025
899aba5
Remove redundant varargs in getbc for ContinuousBoundaryFunction
bgroenks96 Sep 26, 2025
f100b7e
Merge branch 'bg/fix-discrete-bc' of github.com:CliMA/Oceananigans.jl…
simone-silvestri Sep 29, 2025
5fc0ff0
Merge branch 'main' into bg/fix-discrete-bc
simone-silvestri Sep 29, 2025
027aa8f
Revert "Merge branch 'bg/fix-discrete-bc' of github.com:CliMA/Oceanan…
bgroenks96 Oct 2, 2025
099c081
Merge branch 'main' into bg/fix-discrete-bc
bgroenks96 Oct 2, 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
12 changes: 6 additions & 6 deletions src/BoundaryConditions/continuous_boundary_function.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ const ZBoundaryFunction{LX, LY, S} = ContinuousBoundaryFunction{LX, LY, Nothing,

# Return ContinuousBoundaryFunction on east or west boundaries.
@inline function getbc(cbf::XBoundaryFunction{LY, LZ, S}, j::Integer, k::Integer,
grid::AbstractGrid, clock, model_fields, args...) where {LY, LZ, S}
grid::AbstractGrid, clock, model_fields) where {LY, LZ, S}
Copy link
Member

Choose a reason for hiding this comment

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

I suggest keeping the original behavior so that future model developers can support more arguments?

Copy link
Member

Choose a reason for hiding this comment

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

Unless we can articulate why removing this aspect of the code design is undesirable.

Copy link
Collaborator

@simone-silvestri simone-silvestri Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If we want this, we need to pass the args... to the continuous boundary function, which is not the original behavior. If we want to support more arguments, there is some structural work to be done on the ContinuousBoundaryFunctions to make sure that field dependencies do not conflict with the extra arguments passed to the getbc.

It might be easier to allow this only for a DiscreteBoundaryFunction.

Copy link
Member

Choose a reason for hiding this comment

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

Here, we want to have a generic args... so that a model implementation can pass additional arguments into fill_halo_regions! (which then are propagated into getbc). However, having args... does not imply that ContinuousBoundaryFunction has to support interpreting it. ContinuousBoundaryFunction can have an interface which only utilizes the first two arguments clock, model_fields and discards the rest. Then the extra args... are only used by DiscreteBoundaryFunction.


i, i′ = domain_boundary_indices(S(), grid.Nx)
args = user_function_arguments(i, j, k, grid, model_fields, cbf.parameters, cbf)
Expand All @@ -135,7 +135,7 @@ end

# Return ContinuousBoundaryFunction on south or north boundaries.
@inline function getbc(cbf::YBoundaryFunction{LX, LZ, S}, i::Integer, k::Integer,
grid::AbstractGrid, clock, model_fields, args...) where {LX, LZ, S}
grid::AbstractGrid, clock, model_fields) where {LX, LZ, S}

j, j′ = domain_boundary_indices(S(), grid.Ny)
args = user_function_arguments(i, j, k, grid, model_fields, cbf.parameters, cbf)
Expand All @@ -146,7 +146,7 @@ end

# Return ContinuousBoundaryFunction on bottom or top boundaries.
@inline function getbc(cbf::ZBoundaryFunction{LX, LY, S}, i::Integer, j::Integer,
grid::AbstractGrid, clock, model_fields, args...) where {LX, LY, S}
grid::AbstractGrid, clock, model_fields) where {LX, LY, S}

k, k′ = domain_boundary_indices(S(), grid.Nz)
args = user_function_arguments(i, j, k, grid, model_fields, cbf.parameters, cbf)
Expand All @@ -161,7 +161,7 @@ end

# Return ContinuousBoundaryFunction on the east or west interface of a cell adjacent to an immersed boundary
@inline function getbc(cbf::XBoundaryFunction{LY, LZ, S}, i::Integer, j::Integer, k::Integer,
grid::AbstractGrid, clock, model_fields, args...) where {LY, LZ, S}
grid::AbstractGrid, clock, model_fields) where {LY, LZ, S}

i′ = cell_boundary_index(S(), i)
args = user_function_arguments(i, j, k, grid, model_fields, cbf.parameters, cbf)
Expand All @@ -172,7 +172,7 @@ end

# Return ContinuousBoundaryFunction on the south or north interface of a cell adjacent to an immersed boundary
@inline function getbc(cbf::YBoundaryFunction{LX, LZ, S}, i::Integer, j::Integer, k::Integer,
grid::AbstractGrid, clock, model_fields, args...) where {LX, LZ, S}
grid::AbstractGrid, clock, model_fields) where {LX, LZ, S}

j′ = cell_boundary_index(S(), j)
args = user_function_arguments(i, j, k, grid, model_fields, cbf.parameters, cbf)
Expand All @@ -183,7 +183,7 @@ end

# Return ContinuousBoundaryFunction on the bottom or top interface of a cell adjacent to an immersed boundary
@inline function getbc(cbf::ZBoundaryFunction{LX, LY, S}, i::Integer, j::Integer, k::Integer,
grid::AbstractGrid, clock, model_fields, args...) where {LX, LY, S}
grid::AbstractGrid, clock, model_fields) where {LX, LY, S}

k′ = cell_boundary_index(S(), k)
args = user_function_arguments(i, j, k, grid, model_fields, cbf.parameters, cbf)
Expand Down
24 changes: 13 additions & 11 deletions src/BoundaryConditions/discrete_boundary_function.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,23 @@ A wrapper for boundary condition functions with optional parameters.
When `parameters=nothing`, the boundary condition `func` is called with the signature

```
func(i, j, grid, clock, model_fields)
func(i, j, grid, clock, model_fields, args...)
```

where `i, j` are the indices along the boundary,
where `grid` is `model.grid`, `clock.time` is the current simulation time and
`clock.iteration` is the current model iteration, and
`model_fields` is a `NamedTuple` with `u, v, w`, the fields in `model.tracers`,
and the fields in `model.diffusivity_fields`, each of which is an `OffsetArray`s (or `NamedTuple`s
of `OffsetArray`s depending on the turbulence closure) of field data.
`grid` is `model.grid`,
`model_fields` is a `NamedTuple` with `u, v, w`, the fields in `model.tracers` or
the fields in `model.diffusivity_fields`, each of which is an `OffsetArray`s (or `NamedTuple`s
of `OffsetArray`s depending on the turbulence closure) of field data,
and `args` are any additional arguments passed to `getbc`.
Note also that `clock.time` is the current simulation time and `clock.iteration` is the current model
iteration.

When `parameters` is not `nothing`, the boundary condition `func` is called with
the signature

```
func(i, j, grid, clock, model_fields, parameters)
func(i, j, grid, clock, model_fields, parameters, args...)
Copy link
Member

Choose a reason for hiding this comment

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

wrt to the above comment, this docstring indeed would be misleading for any model that chooses to use arguments other than clock, model_fields.

Copy link
Member

Choose a reason for hiding this comment

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

if such models are developed we may want to replace this with a comment that the function signature should refer to the model documentation.

```

*Note* that the index `end` does *not* access the final physical grid point of
Expand All @@ -34,17 +36,17 @@ end
const UnparameterizedDBF = DiscreteBoundaryFunction{<:Nothing}

@inline getbc(condition::UnparameterizedDBF, i::Integer, j::Integer, grid::AbstractGrid, clock, model_fields, args...) =
condition.func(i, j, grid, clock, model_fields)
condition.func(i, j, grid, clock, model_fields, args...)

@inline getbc(condition::DiscreteBoundaryFunction, i::Integer, j::Integer, grid::AbstractGrid, clock, model_fields, args...) =
condition.func(i, j, grid, clock, model_fields, condition.parameters)
condition.func(i, j, grid, clock, model_fields, condition.parameters, args...)

# 3D function for immersed boundary conditions
@inline getbc(condition::UnparameterizedDBF, i::Integer, j::Integer, k::Integer, grid::AbstractGrid, clock, model_fields, args...) =
condition.func(i, j, k, grid, clock, model_fields)
condition.func(i, j, k, grid, clock, model_fields, args...)

@inline getbc(condition::DiscreteBoundaryFunction, i::Integer, j::Integer, k::Integer, grid::AbstractGrid, clock, model_fields, args...) =
condition.func(i, j, k, grid, clock, model_fields, condition.parameters)
condition.func(i, j, k, grid, clock, model_fields, condition.parameters, args...)

# Don't re-convert DiscreteBoundaryFunctions passed to BoundaryCondition constructor
BoundaryCondition(Classification::DataType, condition::DiscreteBoundaryFunction) = BoundaryCondition(Classification(), condition)
Expand Down
7 changes: 2 additions & 5 deletions src/Models/HydrostaticFreeSurfaceModels/compute_hydrostatic_flux_bcs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,14 @@ function compute_flux_bc_tendencies!(model::HydrostaticFreeSurfaceModel)
velocities = model.velocities
tracers = model.tracers

args = (model.clock, fields(model), model.closure, model.buoyancy)


# Velocity fields
for i in (:u, :v)
@apply_regionally compute_flux_bcs!(Gⁿ[i], velocities[i], arch, args)
@apply_regionally compute_flux_bcs!(Gⁿ[i], velocities[i], arch, model.clock, fields(model))
end

# Tracer fields
for i in propertynames(tracers)
@apply_regionally compute_flux_bcs!(Gⁿ[i], tracers[i], arch, args)
@apply_regionally compute_flux_bcs!(Gⁿ[i], tracers[i], arch, model.clock, fields(model))
end

return nothing
Expand Down
Loading