-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AbstractOperations for diagnostics (and beyond...) #463
Conversation
…ield operator functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand or appreciate the AbstractOperations
module yet but I'm hoping to play around and experiment with it over the next few weeks and find out what I like and if I find anything that could be improved.
Definitely poised to become a killer feature I think!
More or less happy to merge this as-is because every other PR will introduce merge conflicts at this point, and it would be good to keep things moving. I think the best way to review a feature this big is to just start using it. We should probably tag v0.14.0 once this is merged.
Most of my comments were related to adding more explanation in the form of comments and docstrings so that mere mortals can understand the code.
The PR introduction you wrote up could serve quite well as a first stab at documenting this feature.
The two_dimensional_turbulence.jl
example is great at showcasing abstract operations!
Pedantic comment but apparently the "Wikipedia sanctioned" name for n-ary operations might be polyadic, multary, or multiary: https://en.wikipedia.org/wiki/Arity#Other_names
Questions:
- Some of the abstract operations tests are CPU-only (the non-computation ones?). Would it make sense to run them on the GPU as well?
- It seems that
model.pressures.pHY′
has taken on the role of being the temporary array. Can't think of a great way of doing temporary arrays without using up extra memory but maybe it makes sense to define a function so it's easy to change the temporary array?
temporary_array(model) = model.pressures.pHY′
Some ideas for next steps:
- Performance benchmarking (will likely be quite extensive to cover many possible use cases).
- Will probably need quite extensive documentation so that users can make full use of abstract operations.
Note: I made comments along the way with the understanding that I might answer my own question by the time I finish reviewing this PR. I'll make a second pass through my comments and resolve the ones that I ended up answering.
function interpolation_operator(from, to) | ||
x, y, z = (interpolation_code(X, Y) for (X, Y) in zip(from, to)) | ||
|
||
if all(ξ === :a for ξ in (x, y, z)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious why ===
instead of just ==
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I figure if ===
works then its best because its more explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that ===
is the fallback for ==
when ==
is not implemented (presumably ==
is mostly needed for numeric comparisons?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: ==
is also used for strings.
@@ -136,11 +136,12 @@ Abstract supertype for fields stored on an architecture `A` and defined on a gri | |||
abstract type AbstractField{A, G} end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is AbstractField{A, G}
still used or should we just define AbstractLocatedField{X, Y, Z, A, G}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a design question. At the moment we do not need both because all subtypes of AbstractField
are also subtypes of AbstractLocatedField
. Thus functionally we could nuke AbstractLocatedField
and change the definition of AbstractField
to carry around X, Y, Z
.
The only concern is that at some point in the future we may actually want the concept of an "unlocated" field; in that case we would regret such a change. Some "fields" like a constant value, may not naturally have a "location". On the other hand, the field arithmetic introduced in this PR requires a field to have a location; thus even a constant field would have to define a nominal location to participate in abstract operations, even though that location would have no effect on operations.
In summary: probably ok to delete AbstractLocatedField
. But maybe save for future PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's only a one-liner so seems find to keep it the way it is right now.
I agree maybe there will be a use for it in the future (although that's probably not a good reason for keeping around unused code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such defensive code design is not altogether terrible. It's used in GPUifyLoops for example (the abstract type GPU
with one subtype, CUDA
).
We do actually utilize AbstractField
to make dispatching on A
a bit easier, since getting to A
in AbstractLocatedField
requires wading through X, Y, Z
.
No. I have no idea why that happened. What can we do to remove them? |
I didn't spend enough time researching! Ok, let's change |
We can. Such a test may end up running on the CPU via scalar operations though... ?
That's a good idea for sure to permit dispatch on various type parameters of Note that
I think just a few will suffice for shallow and deep operations trees, perhaps choosing common use cases to ensure that using abstract operations rather than hard-coded kernels doesn't result in a big performance hit. It will be hard to interpret the results of a benchmark on a deep tree anyways, because we won't have an alternate implementation to compare against. Future performance optimization could use some kind of tree analysis utility + shared memory to accelerate kernels. |
Why extensive? I'm just not sure what to write: the rules for how things work are already all there in the docstrings. Maybe examples are what's needed? |
Looks like you already did. I would usually try something like
Ah ok, maybe not a great idea then as it would slow testing down. I guess the computations are tested on the GPU which is good enough.
Ah nice. I guess I was thinking in case
Hmmm, I was thinking it would be good to benchmark each operator at least once but I suppose if Shallow and deep trees makes sense. True we may not have an alternative implementation but we can compare the deep and shallow tree computations to get an idea. I find comparing the computation time to the time per iteration (~30 ms for 256^3) to be helpful.
Didn't mean to suggest that we need extensive documentation right away. Having examples of what's possible will be really useful, but we can build up a collection of good examples over time. PS: Think you missed half my comments as GitHub collapsed and hid them, but it was mostly minor comments anyways. |
I think the proper benchmark is to compare a unary operation via one function of our choice (say, |
When I said "we don't have an alternative implementation" I should have actually said that for a "deep" tree, an alternative implementation to an abstract operation would be difficult to code. We definitely do have alternative implementations to abstract operations --- we can simply hand-code kernels as we have done many times in the past. The hand-coded kernel is the standard against which we should compare an abstractly-defined kernel. |
The main confusion about abstract operations is not actually the abstract operations themselves, but the way that julia parses infix operators. Would it be clearer if I told you that you can construct an operation tree by writing a mess of nested functions like kinetic_energy = /(+(^(u, 2), ^(v, 2), ^(w, 2)), 2) This is what happens when you write We can document this of course but this is not our code. This is base julia. I understand that it can be mysterious if you are not completely comfortable with how infix operators translate to ordinary function calls. Thus we can demonstrate usage through examples? But there's not much documentation to write. We've define a function |
Sorry, that was my bad. This makes sense. |
|
||
# Ordinary functions needed for fields | ||
architecture(::FunctionField) = nothing | ||
data(f::FunctionField) = f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, are you using data
to represent a function's operation tree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is just dispatch because we need a way to get at field.data
. It's similar to how Base.parent
works. Base.parent
extracts the "parent' array when called on a wrapper around an array, or the array itself if called on an unwrapped array.
|
||
"Returns a view over the interior points of the `field.data`." | ||
@inline data(f::AbstractField) = view(f.data, 1:f.grid.Nx, 1:f.grid.Ny, 1:f.grid.Nz) | ||
@inline interior(f::Field) = view(f.data, 1:f.grid.Nx, 1:f.grid.Ny, 1:f.grid.Nz) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interior
is not an intuitive term for data held by an object. Is this change required because data
is used to represent a function's operation tree (in function_fields.jl)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more mundane than that. The function data
now returns field.data
for field::Field
, and the object itself for an AbstractOperation
. We use this because while AbstractOperation
can be used on the GPU, a Field
cannot and so we need to extract the underlying OffsetArray
from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interior
returns the grid points in the model "interior" (ie, excluding halos).
What would be more intuitive?
Hopefully in the future we can use dispatch on Colon
(see #458) so that the user can ignore these functions altogether. I think we should strive for syntax that allows the user to think of a Field
in the way one would think of a mathematical continuous field, rather than having to interact with the underlying discrete data and such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function data now returns field.data for field::Field, and the object itself for an AbstractOperation
I would prefer values
or data
. If data
works, why are you using interior
? Does data
return halos as well? I was just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
values
is a julia Base
function: for Dict
-like objects it returns the values (see also the function keys
)...
Maybe interior_values
?
For field::Field
, data(field)
returns field.data
, which is an OffsetArray
. interior
returns a view over the same offset array, which excludes the halo points. The difference only matters for broadcasting an operation over an array. For example, if you have an array of values you would like to set the solution to, then
field.data .= user_array
will fail, unless user_array
includes halo points. This is undesirable because halos are arbitrary. Thus they would use
interior(field) .= user_array
which due to the view, will work when user_array
contains only interior values.
However, I think we should encourage users to use our functions rather than manually setting values, as in:
set!(field, user_array)
For plotting interior
would also prove useful if you don't want to plot halos. Almost always, however, we are plotting slices (not the whole array); in that case dispatch on Colon
may suffice since we can design getindex
such that
field[:, 1, :]
returns a slice over interior values, excluding halos.
AbstractOperations for diagnostics (and beyond...) Former-commit-id: 20f9d08
Introduction
This PR introduces
AbstractOperation
s for constructing 3D expressions with a friendly syntax (eg, not writing kernels), which can then be evaluated online during a simulation for diagnostics / output purposes.This PR introduces four kinds of
AbstractOperation
s:Derivative
(things like∂x
,∂y
)BinaryOperation
(things likea-b
,a^b
)UnaryOperation
(things likesqrt
,sin
,cos
)PolynaryOperation
(things likea + b + c + d
ora * b * c * d
)All of these act on combinations of
Field
,Function
, andNumber
. `AbstractOperation
s arise from what appears to be ordinary arithmetic and calculus performed onField
s. Consider this example:Here, the object
a_times_b
is of the typeBinaryOperation
.a_times_b
can be indexed into like an array or field, however:Staggered grids, interpolation, and operation "location"
Operations can also be defined between fields at different locations in the staggered grid:
Note that the location of an operation defaults to the location of the first field involved.
This submodule also exports a macro
@at
for specifying the location of an operation. Therefore we can writekinetic_enegy
has a deep nested structure that is difficult to interrogate. Therefore one speculative possible item to add to this PR is to write ashow
method forOperations
, using some kind of tree visualization?For the purposes of this PR description, we hack together a visualization of the structure of
kinetic_energy
:We see that
kinetic_energy
, at the top level, isBinaryOperation
between aPolynaryOperation
and an integer (2), involving division/
(which is indeed, what we wrote).Delving into the
PolynaryOperation
one level down,we find that it consists of a sum of three
BinaryOperations
. Finally, we see that eachBinaryOperation
,involves taking an
OffsetArray
(each of which holds the underlying data inu, v, w
) to the power 2. Still, we index into it in the same way we index into other fields to obtain its data:Special considerations
There are a few special rules to how operations are handled:
A
BinaryOperation
between two fields at the same location is always performed at their common location;a
BinaryOperation
between a field and a number always takes place at the location of the field.These special rules override the specification of operator location via
@at
. Thus, for example, in the operationthe product
u*v
is computed at the cell center, whilev^2
is computed at thev
-pointCell, Face, Cell
, and afterwards interpolated to cell centers. This functionality is achieved by endowingBinaryOperation
with three interpolation operators: two interpolation operators applied to each field prior to interpolation, and an interpolation operator that is applied to the result. Special cases can then be handled by defining operators for cases in which a computation should be performed at the location of one or both of the fields.Furthermore, we do not provide
PolynaryOperation
s with default locations. In other words, aPolynaryOperation
only arises when location is specified, eg:This also means that all fields in a
PolynaryOperation
are interpolated prior to computation.Computations
This PR also adds a type for making "computations" with an
AbstractOperation
. We call this aComputation
. From the tests:which launches a 3D kernel to compute the result of an
AbstractOperation
at every grid point, storing the result incomputation.result
(which here has been defined asmodel.pressures.pHY′
).We also extend
HorizontalAverage
to work withComputation
, (and add a constructor that constructs aComputation
from anAbstractOperation
andModel
), egCalling
run_diagnostic
forHorizontalAverage{<:Computation}
first callscompute!(computation)
, and then compute the horizontal average ofcomputation.result
.Some important details
This PR changes the meaning of
data
, which is used extensively in forming operations. This resolves Behavior ofdata
is inconsistent with behavior ofBase.parent
#454. A new functioninterior
serves the original purpose ofdata
.This PR depends on the arbitrary tracers PR. The arbitrary tracers PR should be merged before this one.
To permit mixing
Function
s in withField
s andNumber
s inAbstractOperations
, we also define aFunctionField
type that acts like a field, but invokes a function to compute values at its location under the hood. This type probably needs more tests. We should also investigate whether this type can be used to specify things like background fields. AFunctionField
can either be time-dependent and a function ofx, y, z, t
(achieved by allowing it to possess a reference toclock
), or a static function ofx, y, z
.This PR changes the result of a horizontal average to
HorizontalAverage.result
to provide a common terminology withComputation
, as well as future reductions along other dimensions (previously the result of a horizontal average was calledprofile
)This PR adapts
Field
to work on the GPU (after the hard work of adapting all theAbstractOperation
s to work on the GPU, this seemed trivial). That this works now should be tested. If it does indeed work and there is no loss of performance, we can eliminate a lot of cruft from ourtime_step!
, and also use fields directly inAbstractOperation
s (right now we extract the underlyingOffsetArray
instead).In summary
Miraculously, tests pass on the GPU. However, this framework is quite general and powerful, so we need to
And certainly before merging we also need
Also, it'd be nice to
AbstractOperation
to define useful output in an example.Some discussion may be warranted about what's exported from
AbstractOperations
; we almost always want to haveFace
andCell
, for example. Ultimately, I think it'd be nice to writeusing Oceananigans.Fields, Oceananigans.AbstractOperators
to get what's needed for this purpose. But that's yet another future PR.I hope this sparks some discussion about the future of the
Field
abstraction as well. I think it has the potential to be quite powerful.Resolves #454
Resolves #428