Oceananigans.jl icon indicating copy to clipboard operation
Oceananigans.jl copied to clipboard

Fix `BoundaryAdjacentMean`

Open jagoosw opened this issue 8 months ago • 3 comments

Addresses concerns in #4183 and bug

jagoosw avatar Mar 10 '25 13:03 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!.

glwagner avatar Mar 11 '25 03:03 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!`.

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.

jagoosw avatar Mar 11 '25 14:03 jagoosw

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) ?

glwagner avatar Mar 11 '25 15:03 glwagner