Boundary condition ordering

[simone-silvestri]

At the moment, the fill halo regions follow a particular ordering where

Flux, Value, Gradient > Periodic
Periodic > Halo Communication

where > indicates the priority of execution.
We also fill the two sides of one direction together.
This execution order cannot be respected in case:

bc.west isa Periodic
bc.east isa Periodic
bc.south isa Flux
bc.north isa DistributedCommunication

The possible solutions are two:

• eliminate the order requirements between Flux, Value, Gradient and Periodic by including corners in all local fill_halo_regions! (at the moment only Periodic fills the corners)
• do not fill two sides together

probably the first solution is better because it leads to simpler code both in terms of actual implementation and in terms of logic of execution

[simone-silvestri]

Another solution, slightly more complex but that allows us to keep the current execution model is to separate the sides only on communicating boundary conditions

[glwagner]

Ok, and to clarify, the issue is cropping up now because we are trying to distribute a model / grid along a Bounded direction. If we only distribute in Periodic directions, this problem never crops up. But if we distributed in a Bounded direction, then we run into the issue where one "side" may have a bounded condition like flux, value, gradient, and the other side may be communicating.

If we don't consider distributed problems, this issue never occurs: either both sides are Periodic, or bounded.

[simone-silvestri]

exactly this is the issue with communication that pops up also in shared cases (multi-region)

[glwagner]

So how do you eliminate ordering requirements? (Why do we have ordering requirements in the first place?)

[simone-silvestri]

ordering requirements are necessary for filling corner halos.
This is done by Periodic boundary conditions in non-distributed simulations.
Additionally, since communication boundary conditions can be asynchronous, distributed (and multi-region) BCS need to be filled last.

To remove order requirements we would need to fill the halo for flux, value, and gradient also in the corners.
I thought that might be a good idea but we hit a problem when having an AbstractArray boundary condition because we would need to construct the associated OffsetArray.

This can be prevented by wrapping the array in a Field and filling the halo regions but it seems like a heavy requirement to do it, and generally, a large API change that we might want to think about a little more. In #3338 I fixed the problem by separating out communicating boundary conditions which wasn't that complicated and maintained the current logic.

Maybe in the future, we might want to eliminate the order requirement though. So we can keep this issue open.

[glwagner]

Why don't we discontinue support for array boundary conditions and only support Field?

[glwagner]

It's better practice to use Field, because then user scripts are more likely to port to GPU and distributed architectures. I think we want to discourage using Array --- if it makes development easier, we might as well discontinue support altogether.

[simone-silvestri]

This was fixed in #3338