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

Changes from 1 commit
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