Fix getbc not passing extra args for discrete BC functions

[bgroenks96]

This is a quick fix for what seems to be a bug in the implementation of getbc for DiscreteBoundaryFunctions. The extra args are not passed through to the function as they are for ContinuousBoundaryFunction.

Let me know @glwagner and/or @simone-silvestri if this is actually intentional and I am missing something.

[simone-silvestri]

I think this is intentional, i.e. the signature of a discrete boundary condition function is

@inline discrete_bc_function(i, j, grid, clock, fields, parameters)

where fields is a namedtuple containing fields(model) (which should include diagnostic and prognostic variables) and parameters is passed to the boundary condition upon construction

discrete_bc = FluxBoundaryCondition(discrete_bc_function, discrete_form=true, parameters=my_parameters)

[simone-silvestri]

The extra args... for a ContinuousBoundaryFunction are there because users can specify field dependencies, for example:

continuous_bc_function(x, y, z, t, u, v) = ....

where, in this case, u and v are the velocity fields.
At this point the constructor will be

continuous_bc = FluxBoundaryConditions(continuous_bc_function, field_dependencies = (:u, :v))

note that the model must contain :u and :v within fields(model). As the field dependencies can be none or any other number of fields, we pass an args... to the getbc method. This is not required in a discrete boundary function because we are explicitly passing fields(model) to the boundary condition

[simone-silvestri]

Actually, now that I look more in detail, I think the ContinuousBoundaryFunctions have a redundant args... at the end...

[bgroenks96]

Oh, ok. But then what is the point of having the extra args... in fill_halo_regions!?

[simone-silvestri]

To pass clock, fields, we don't always pass those.
Basically we have two modes for fill_halo_regions!:

fill_halo_regions!(field)

for fields that do not have field dependencies and

fill_halo_regions!(field, model.clock, fields(model))

for fields that do.

[bgroenks96]

Ok I updated the PR to remove the reundant args then.

[bgroenks96]

@simone-silvestri Do you know what's going on with the tests?

[simone-silvestri]

Apparently those args... where needed somewhere in the tests. I ll take a look tomorrow

[simone-silvestri]

Not sure why we were passing also closures and buoyancy in the hydrostatic model that were eventually thrown away...
The tests should pass now

[glwagner]

Just to add here --- the design is such that any extra arguments to fill_halo_regions! are propagated into boundary condition functions. It isn't necessary for these extra arguments to be clock, model_fields. The arguments can be anything that a model developer wishes to choose --- they just have to pass the arguments to fill_halo_regions! and then document to users what is expected within the boundary condition functions.

NonhydrostaticModel and HydrostaticFreeSurfaceModel have chosen to use an interface with clock, model_fields.

[glwagner]

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

[glwagner]

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

[simone-silvestri]

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.

[glwagner]

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.

[glwagner]

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

[glwagner]

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

[bgroenks96]

Just to add here --- the design is such that any extra arguments to fill_halo_regions! are propagated into boundary condition functions.

Great, that's what I originally had assumed and also what I wanted!

Does this mean that the original change was the correct one?

i.e. fcdecfb

[simone-silvestri]

Not really, a ContinuousBoundaryFunction does not allow extra arguments out of two extra arguments passed to fill_halo_regions! (which for both continuous and discrete boundary function necessarily need to be something resembling a clock and a namedtuple which contains some fields). Note how args... is replaced by field dependencies in line 130, and time is retrieved from the clock and passed to the function in

Oceananigans.jl/src/BoundaryConditions/continuous_boundary_function.jl

Lines 126 to 134 in c75a112

	@inline function getbc(cbf::XBoundaryFunction{LY, LZ, S}, j::Integer, k::Integer,
	grid::AbstractGrid, clock, model_fields, args...) where {LY, LZ, S}
	i, i′ = domain_boundary_indices(S(), grid.Nx)
	args = user_function_arguments(i, j, k, grid, model_fields, cbf.parameters, cbf)
	X = x_boundary_node(i′, j, k, grid, LY(), LZ())
	return cbf.func(X..., clock.time, args...)
	end

Therefore, a generic number of arguments is not supported (at the moment), neither for continuous nor for discrete boundary functions. Allowing it for a DiscreteBoundaryFunction is quite simple and requires just what has been done originally in this PR. However, this very simple change still relies on some assumptions: the first two extra arguments must be (1) a struct containing a time field in position 1 and (2) a namedtuple in position 2. Another subtlety would be deciding at what position the parameters would appear in the function signature, probably at position number 3?
Allowing a generic number of inputs for a ContinuousBoundaryFunction might require more work than this PR because field dependencies are splatted in the continuous function, so we would need to decide whether the extra arguments come before or after the field dependencies in the function signature.

To be honest, I like the simple design of passing a time (clock), field dependencies (fields), and some parameters that hold other user-defined arguments. It makes the interface simple, unique, and easily documentable for any boundary condition. I also think it covers most (if not all) of the use cases. However, if this design is not sufficient for other models' needs, we could expand it. @bgroenks96, what is the pattern that you need to achieve?

Adding args... at the end might be a simple enough change to just cover more cases for both the countinuous and discrete boundary functions

[simone-silvestri]

In general, the idea to answer before starting to change things is: how do you want your function signature to look for a generic (continuous or discrete) boundary condition that can be passed to your model?

[glwagner]

It seems like a fairly useful situation would be one in which the boundary conditions depend on a diffusivity or viscosity. This has been requested a number of times for NonhydrostaticModel but we haven't been able to support it because adding the closure auxiliary fields to model_fields causes type inference failure in many cases. Other model types could be more successful though and able to support it.

Note how args... is replaced by field dependencies in line 130

Can we change this so the code isn't so confusing to read?

[glwagner]

Let me add a few points here:

1. I believe that the arguments to HydrostaticFreeSurfaceModel are used by vertical mixing scheme boundary conditions. For example computing the TKE flux for k-epsilon requires knowledge of some closure parameters. This boundary condition is not part of the user interface, it is added automatically within the model constructor.

2. More generally, it is nice if model developers can pass additional arguments into fill_halo_regions! to support more general user interfaces than supported by NonhydrostaticModel and HydrostaticFreeSurfaceModel.

3. I also think that we should keep the user interface for ContinuousBoundaryFunction fixed; ie in the case that models provide more arguments to fill_halo_regions! than just clock, model_fields, those extra arguments are ignored by ContinuousBoundaryFunction (they can only be accessed by discrete form boundary conditions).

[glwagner]

@bgroenks96 can you state the aim of this PR (not just its literal content)? Ie what outcome is it trying to achieve.

[bgroenks96]

I had the idea to pass additional arguments to fill_halo_regions! and compute_flux_bcs! (specifically, a named tuple of input fields) and wanted to see how that would work. When I looked at the code more carefully, I noticed this discrepancy (extra args not being passed into DiscreteBoundaryFunction) and figured it was a mistake.

Edit: Also, I am fine with proposed design of passing arbitrary additional args only to DiscreteBoundaryFunction. The discrete form would be anyway better for the case I referred to above, since ContinuousBoundaryFunction by design needs to interpolate field dependencies.

[bgroenks96]

Ah I see, this breaks GPU support because the closure and buoyancy are passed into getbc... I didn't realize this was also a problem for the discrete case.

I don't think this is a super critical change (I have been able to make do without it so far), so maybe we could just add a note to the docstring of DiscreteBoundaryFunction and/or getbc that the additional args... are ignored by default?

[glwagner]

I had the idea to pass additional arguments to fill_halo_regions! and compute_flux_bcs! (specifically, a named tuple of input fields) and wanted to see how that would work. When I looked at the code more carefully, I noticed this discrepancy (extra args not being passed into DiscreteBoundaryFunction) and figured it was a mistake.

Edit: Also, I am fine with proposed design of passing arbitrary additional args only to DiscreteBoundaryFunction. The discrete form would be anyway better for the case I referred to above, since ContinuousBoundaryFunction by design needs to interpolate field dependencies.

Ok, now I see with the latest code changes what is needed! I think this is a positive addition --- thank you @bgroenks96

There are two additional changes that I think are needed, which I am happy to add here. First, we need to clean up / change HydrostaticFreeSurfaceModel so that the additional closure, buoyancy arguments (needed for CATKE) are only added when CATKE is being used.

Also, because DiscreteBoundaryFunction is more generic, I think we can delete / clean up some of the code that is currently part of CATKE's tke boundary condition implementation.

Happy to take this on @bgroenks96 if you are ok with me sending a few commits here.

[glwagner]

Ah I see, this breaks GPU support because the closure and buoyancy are passed into getbc... I didn't realize this was also a problem for the discrete case.

I think the problem is that the user interface for HydrostaticFreeSurfaceModel is relying on DiscreteBoundaryFunction throwing away the additional arguments. This is a hack --- the proposed change cleans this up --- but the clean up also requires HydrostaticFreeSurfaceModel to clean up its act since it can't use the hack anymore. Which is good I think.

I don't think this is a super critical change (I have been able to make do without it so far), so maybe we could just add a note to the docstring of DiscreteBoundaryFunction and/or getbc that the additional args... are ignored by default?

I'm ok to take this up and push it through if you like! I understand the objective now --- allow args to propagate into DiscreteBoundaryFunction, and refactoring HydrostaticFreeSurfaceModel to support CATKE's boundary condition given this behavior.

[bgroenks96]

Feel free to push those changes! Thanks!