Fix GPU indexing flat fields

[simone-silvestri]

solves #4044

cc @jagoosw

TODO:

• add a test

[jagoosw]

Lgtm, thank you!

[jagoosw]

Lgtm, thank you!

[glwagner]

Are we sure we want this? Implementation is obvious but the real question is whether we want. I believe this was omitted by design to clarify reading code.

[jagoosw]

Are we sure we want this? Implementation is obvious but the real question is whether we want. I believe this was omitted by design to clarify reading code.

Fine with me either way, its just a bit strange when you're writing code that is only ever going to see a 2D field (e.g. a flat sediment), and you have to index with three indices.

[glwagner]

Are we sure we want this? Implementation is obvious but the real question is whether we want. I believe this was omitted by design to clarify reading code.

Fine with me either way, its just a bit strange when you're writing code that is only ever going to see a 2D field (e.g. a flat sediment), and you have to index with three indices.

How can someone who is reading the code know which dimensions are being referred to? Because I'm assuming that even though the field is 2D, it is being used in a 3D model right?

[simone-silvestri]

As I see it we are undecided here between two options:

(1) the "right thing": index lower dimensional fields with the appropriate amount of indices; i.e. index a field that has Nothing in the z-direction with [i, j]. This is the "right thing" because it is logically correct as any additional index k would be redundant. Note that also indexing with [i, j, 1] is logically incorrect because a 2D field does not "live" on index 1 in z, and (especially if the model is 3-dimensional!), this might lead to confusion. Personally, I was always indexing 2D fields with 2 indices, and I see also @jagoosw and @seamanticscience doing that.

(2) the "convenient thing", i.e. just treat lower dimensional fields as 3D because they have an underlying 3D data structure, but throw away not relevant indices. This approach leads to less code in the source as an underlying assumption that we can always index a field with three-dimensional indices simplifies the source code notably. This might come at the cost of logical inconsistency (where does the 2D field live in the third dimension?).

At the moment Oceananigans implements (2) but until recently also (1) was enabled involuntarily (probably not correctly though). Option (2) was then disabled when changing the axes function for fields. This PR enables both again so probably it's not the correct approach? I am not sure what is best, I am in favor of option (1) but I am not sure how unconvenient the source code would become (probably AbstractOperations would become much more complicated). So I would not be in favor of increasing the complexity of the source code too much to enable this.

I think this is a bit of a repeat of the issue we faced with the nodes function, where in the end, we decided to go for option (1)

Thoughts?

[glwagner]

I guess I have appreciated the 3D index in the source code before as a something that yields clarity. The data structures are 3D, actually. So the indexing is correct if we think of indexing as accessing the underlying data.

I'm not sure the user interface and source share concerns. The user interface communicates a scientific idea, but the source code communicates an algorithm which (at least for the two main models we use) are always 3D. I don't think using k=1 for a 2D field is "wrong", this is exactly how you would have to index the underlying data.

I don't think 2D indexing was exactly "supported" before. It is simply that the third index can be dropped. However for a field which is 2D in xz or yz, it was not previously possible to use a 2D index. In other words this works:

julia> a = rand(3, 2, 1)
3×2×1 Array{Float64, 3}:
[:, :, 1] =
 0.013463  0.434527
 0.464574  0.604448
 0.976298  0.291743

julia> a[3, 2]
0.29174347258976163

but this does not:

julia> b = rand(3, 1, 2)
3×1×2 Array{Float64, 3}:
[:, :, 1] =
 0.17625966493661893
 0.8337469207139488
 0.37983095953695944

[:, :, 2] =
 0.6288634010250888
 0.6770954357421748
 0.7976130302042427

julia> b[3, 2]
ERROR: BoundsError: attempt to access 3×1×2 Array{Float64, 3} at index [3, 2]
Stacktrace:
 [1] getindex(::Array{Float64, 3}, ::Int64, ::Int64)
   @ Base ./essentials.jl:14
 [2] top-level scope
   @ REPL[8]:1

I'm uneasy about divorcing indexing from the underlying arrays.

[simone-silvestri]

right, I guess I have only used 2D indexing only for xy fields where the outcome was correct so I never found encountered the problem.

[simone-silvestri]

I'm not sure the user interface and source share concerns. The user interface communicates a scientific idea, but the source code communicates an algorithm which (at least for the two main models we use) are always 3D. I don't think using k=1 for a 2D field is "wrong", this is exactly how you would have to index the underlying data.

I agree there is a separation of concern, the logical issue I have with indexing at k=1 is the fact that k=1 indicates a precise location in the grid (which might be immersed for example) which is not connected logically to the field we are indexing in. So the developer needs to always be careful when dealing with lower dimensional fields and be well aware between the difference from a windowed field at location k=1 and a reduced field in z.

[simone-silvestri]

Additionally, we are already divorcing indexing from data structure when we allow something like this:

Oceananigans.jl/src/Fields/field.jl

Line 523 in 3fbe265

@propagate_inbounds Base.getindex(r::XReducedField, i, j, k) = getindex(r.data, 1, j, k)

If we would want fidelity we should allow indexing only with i=1 in this case.
But I am not necessarily against maintaining the current status quo because I see it is a convenient approach.

[simone-silvestri]

I guess if we keep option (2) we can convert this PR to add some documentation that details this (maybe a docstring on ReducedField)?

[glwagner]

Additionally, we are already divorcing indexing from data structure when we allow something like this:

Oceananigans.jl/src/Fields/field.jl

Line 523 in 3fbe265

@propagate_inbounds Base.getindex(r::XReducedField, i, j, k) = getindex(r.data, 1, j, k)

If we would want fidelity we should allow indexing only with i=1 in this case.
But I am not necessarily against maintaining the current status quo because I see it is a convenient approach.

Right but in this case, its the caller that has the problem --- the caller wants to use a 3D index but one of the indices is "dead". This is a restriction of how abstract operations are designed so basically the only purpose of that is to support abstract operations...

[simone-silvestri]

I guess we can discuss this in an issue. I have modified this PR to include a clarification in the docs as to why the user interface for setting a field does not correspond to the interface to indexing a field