Missing information in docstring for BoundaryAdjacentMean

[glwagner]

The function signature in the docstring is missing, and the docstring ends with the phrase "Updated by calling"...

Maybe something was accidentally deleted?

Oceananigans.jl/src/Models/boundary_mean.jl

Lines 12 to 15 in d2e9e27

	BoundaryAdjacentMean
	Stores the boundary mean `value` of a `Field`. Updated by calling
	```jldoctest

I was also confused about the purpose of this object. The user interface seems to allow the generation of an operator that can act on any field. But what should this operator used for?

I'm also perplexed by the property names. One property is flux_field. But what does BoundaryAdjacentMean have to do with fluxes?

Oceananigans.jl/src/Models/boundary_mean.jl

Line 51 in d2e9e27

flux_field :: FF

It's also problematic to me that value needs to be a Ref or stored at all. This creates ambiguity about how the operator is supposed to be used, one can either call the operator and store the value manually, or call it without explicitly storing the value. Probably for the sake of clarity it is better not to store the result in a Ref. This will reduce bugs in downstream code by constraining how this feature is used, which will help the community by allowing people to make progress faster.

Can you clean this up @jagoosw ?

[jagoosw]

Not sure what happened with the docstring, I'll open a PR to fix that

[jagoosw]

I was also confused about the purpose of this object. The user interface seems to allow the generation of an operator that can act on any field. But what should this operator used for?

This was meant for use in open boundaries (but I think could be used for other stuff), like:

bam = BoundaryAdjacentMean(grid, :east)

obc = PertubationAdvectionOpenBoundaryCondition(bam)

u_bcs = FieldBoundaryCondition(east = obc)

It's also problematic to me that value needs to be a Ref or stored at all. This creates ambiguity about how the operator is supposed to be used, one can either call the operator and store the value manually, or call it without explicitly storing the value. Probably for the sake of clarity it is better not to store the result in a Ref. This will reduce bugs in downstream code by constraining how this feature is used, which will help the community by allowing people to make progress faster.

We need to store the value otherwise, it would have to be computed at every boundary point, and I don't know how else you would do that (other than a 1-element array)? I guess the other option is to make it a mutable struct and then adapt to a non-mutabel version but that seems un-neccesarily complicated.

I'm also perplexed by the property names. One property is flux_field. But what does BoundaryAdjacentMean have to do with fluxes?

Oceananigans.jl/src/Models/boundary_mean.jl

Line 51 in d2e9e27
flux_field :: FF

This is a good point, I only used it for wall normal velocity so the integral is the wall normal momentum flux, but then when it got renamed to be general for any field flux field should have been changed to just integral or something. I'll change this.

[glwagner]

We need to store the value otherwise, it would have to be computed at every boundary point, and I don't know how else you would do that (other than a 1-element array)?

But it doesn't have to be stored in BoundaryAdjacentMean. It is more properly stored in PertubationAdvectionOpenBoundaryCondition. In other words, operators don't need to store their values. It's only the user of the operator that needs it.

[jagoosw]

PertubationAdvectionOpenBoundaryCondition always uses the condition value for the mean wall normal velocity (which isn't always the value from BoundaryAdjacentMean) so I don't understand how it would store it without being refactored?

I guess we could make some other thing like:

struct ComputedBoundaryVelocity{V, C}
       value::V
    operator::C
end

then we put this in the condition and have an update boundary condition that puts the result of operator into value?

[glwagner]

Why can't condition be a Ref? This matches the pattern we use elsewhere when it has values that need to be updated (for example, when condition is an array).

Where do we compute the value of condition?

[jagoosw]

The condition gets computed in update_boundary_condition!, I don't understand how we would write a method that computes it without having either condition or classifcation be something like BoundaryAdjacentMean (or we could redesign PAOBC).

[glwagner]

My point is that the example:

julia> bam = BoundaryAdjacentMean(grid, :east)
BoundaryAdjacentMean: (0.0)

julia> bam(:east, cf)
-1.5612511283791264e-18

is bad. The example show the creation of an operator bam, which when called with bam(:east, cf) returns a number. Yet inspection of BoundaryAdjacentMean reveals that the function also stores its value. This "double functionality" is a bad design; it generates ambiguity about how to use the operator (users can use it one way, or another), and makes the code harder to understand.

The problem could very simply be solved by using a different syntax like compute!(bam, :east, cf) or something, which forces users into one pattern.

However, I would like to back up and look at the whole workflow that is being implemented, to see if there are additional opportunities to make this code better.

It would help to have links to the code so I don't have to search through the source to find the name of a function like update_boundary_condition!.