Oceananigans.jl: Boundary condition names should be top and bottom for z

[ali-ramadhan]

Right now it's confusing: left means top and right means bottom...

[glwagner]

You can create aliases via const top = right and const bottom = left. Then the user can use top to mean right if they want to, too.

The other solution is to copy and paste every piece of boundary condition code to create a special boundary condition for the z direction. I don't think this makes sense.

My two cents: left and right are the pattern in dedalus, and that's a popular and successful code. The idea of left and right in the context of boundary conditions is mathematical: left is the negative direction, right is the positive direction.

You could also change left and right to positive and negative, I guess.

[glwagner]

Actually sorry, the aliases won't work.

What you'll have to do is overload the setproperty! method to create a special function that interprets bottom as left.

[glwagner]

Wait --- left is top? Is that because of the reverse indexing convention?

[glwagner]

See this post on how you might define a new setproperty! for appropriate types:

https://discourse.julialang.org/t/getproperty-decorations-inheritance-in-0-7/11237/3

[ali-ramadhan]

Yeah currently left is top. I thought the left and right corresponded to the domain [-Lz, 0] and not the indices [1, k] (or [k, 1]), so left should be bottom and right should be top no matter the indexing convention.

I do like the idea of the aliases (or overloading setproperty!). The reason I bring this up is because every time I show the code to someone I always have to explain what left and right are. So I think the code would be more readable if we had the option to say top and bottom.

[glwagner]

The aliases won't work because left and right are properties of a struct.
But overloading setproperty! to catch an attempt to set bc.bottom and convert that to bc.left will work, I think.

Following the post, you'd add the following to boundary_conditions.jl

const CBC = CoordinateBoundaryConditions
Base.setproperty!(cbc::CBC, side::Symbol, bc) = setbc!(cbc, Val(side), bc)

setbc!(cbc::CBC, ::Val{S}, bc) where S = setfield!(cbc, S, bc)
setbc!(cbc::CBC, ::Val{:bottom}, bc) = setfield!(cbc, :left, bc)
setbc!(cbc::CBC, ::Val{:top}, bc) = setfield!(cbc, :right, bc)

[glwagner]

The boundary conditions do depend on / refer to the indexing convention. A boundary condition is applied over a particular region of an array --- not a region in physical space.

The boundary conditions are constructed to apply to x, y, and z. In our abstraction "left" means i=1 for x and y, which makes sense. But because of the reverse convention for z, k=1 is on the right for z.

[ali-ramadhan]

Haha yeah... Unfortunately PR #107 was shelved in favor of more pressing issues.

I still think the indexing shouldn't matter for the notation. I thought left always referred to the "left" end of the domain interval, e.g. _x ∈ [0, Lx]_ so x.left should refer to _x=0_ and x.right should refer to _x=Lx_. Similarly, _z ∈ [-Lz, 0]_ so z.left should refer to _z=-Lz_ while z.right refers to _z=0_. So whether k=1 at the top or bottom doesn't matter to the user and the boundary conditions API (it matters on the backend of course).

PS: Thanks for the code example! Should be easy to add in.

[glwagner]

I feel I'm not making myself clear.

It's not a matter of names or semantics here. It's a matter of the abstraction.

Either the first index means left or not. This is applied uniformly to every direction.

There is only one CoordinateBoundaryConditions for all of x, y, and z.

[ali-ramadhan]

I see. I was describing what it should look like to the user but it sounds like the structs used will need to be refactored to accommodate this change (until PR #107 is finished and merged).

[glwagner]

1. The structs do not need to be refactored if the setproperty method is used.

2. There is only one struct for boundary conditions. What you really mean is that you will create a (new) special struct for z which is in practice identical to the struct for x and y except for the names of it’s fields.

[ali-ramadhan]

True. If we just overload setproperty! as you suggested we'll be good to go here. And then when #107 is merged, we just remember to switch left to right and vice versa in the overloaded function.

[ali-ramadhan]

@glwagner this issue has been resolved in #237 right?