Fix BoundaryAdjacentMean

[jagoosw]

Addresses concerns in #4183 and bug

[glwagner]

should we call this boundary_adjacent_mean to match the type name?

[glwagner]

so the idea is that we compute an integral first and then divide by area to get the mean?

[glwagner]

the type doesn't say anything about "normal" and I think this works on scalars right?

[jagoosw]

That's a good point, I guess boundary_parallel_integral is more accurate? boundary normal integal is actually wrong

[glwagner]

I think merely integral is enough. The name of the type is "boundary adjacent mean". If we compute the mean by first taking the integral and dividing by area, then integral combined with the name of the type give you what yo uneed to know.

[glwagner]

The first rule is "do no harm"

[glwagner]

the function signature is missing arguments

[glwagner]

A few comments:

1. It might be nicer to preform this operation --- that way users can look at it, and potentially verify its correctness.
2. Why don't we use Average here?
3. sum! is incorrect if the grid is stretched, right?

[jagoosw]

Do you mean to return the operation and let the user do it?

I can't exactly remember how I ended up with this design but I think I did it this was so that we don't have to allocate a new average field (since you can't do in place averaging right?)

And I think sum is correct since it is multiplied by the area right?

[glwagner]

I just noticed this. I would offer that this code is surprising and hard to understand.

[glwagner]

I'm no longer sure that we need the type BoundaryAdjacentMean. Can't we construct a Field directly with all of the information it needs, and then call compute! on that field within update_boundary_condition!? What we need is a function boundary_adjacent_mean(side, field) which constructs the appropriate Reduction. getbc then works correctly (I think); for example the needed value is stored in field.data.

A disadvantage is that we need to use regularize_boundary_condition to set it up for users, because users won't have access to field before they build the model.

However, there are some significant advantages:

1. More code re-use; no extra type needed.
2. The way the code works is easier to understand, provided that you already understand reductions.
3. The type is specific to the boundary and its value can be computed without dependencies via compute!(field).
4. Because it is a Field, users can use an output writer to write its value as a diagnostic. It can also participate in additional operations; for example to compute a flux across the boundary.
5. We can simplify update_boundary_condition!.

[jagoosw]

I'm no longer sure that we need the type BoundaryAdjacentMean. Can't we construct a Field directly with all of the information it needs, and then call compute! on that field within update_boundary_condition!? What we need is a function boundary_adjacent_mean(side, field) which constructs the appropriate Reduction. getbc then works correctly (I think); for example the needed value is stored in field.data.

A disadvantage is that we need to use regularize_boundary_condition to set it up for users, because users won't have access to field before they build the model.

However, there are some significant advantages:

1. More code re-use; no extra type needed.

2. The way the code works is easier to understand, provided that you already understand reductions.

3. The type is specific to the boundary and its value can be computed without dependencies via `compute!(field)`.

4. Because it is a `Field`, users can use an output writer to write its value as a diagnostic. It can also participate in additional operations; for example to compute a flux across the boundary.

5. We can simplify `update_boundary_condition!`.

I think the reason I ended up with this was to avoid using regularize_boundary_condition but that seems like a reasonable solution. I think we would also need to add a method for update_boundary_condition! when the BC has a compute field in the condition.

[glwagner]

Ok, so we have to decide which is best: adopt complication of regularize_boundary_condition but simplify what we get out. Probably the main thing is that Field can participate in abstract operations, so if we think that matters we should use it. Also it might be easier to generalize to open boundary velocities that vary in space (eg not a mean over the whole boundary) ?