Skip to content
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

Behavior of data is inconsistent with behavior of Base.parent #454

Closed
glwagner opened this issue Oct 10, 2019 · 3 comments · Fixed by #463
Closed

Behavior of data is inconsistent with behavior of Base.parent #454

glwagner opened this issue Oct 10, 2019 · 3 comments · Fixed by #463

Comments

@glwagner
Copy link
Member

Julia's Base.parent(a) has the behavior that it returns either a.parent where applicable, or just a in the case that a is an unwrapped array.

Our data(f::Field) function has a different behavior: while f does have a field data, the function data(f) actually returns a view into data at interior grid points, rather than data itself.

It might be a good idea to make the behavior of data consistent with that of Base.parent. One way to do this is to change the definition of data to

data(f::Field) = f.data
data(f::AbstractArray) = f

To capture the existing functionality of data, we might define a new function (perhaps interiordata? that returns a view over 'interior' points:

interiordata(f::Field) = view(data(f), 1:f.grid.Nx, 1:f.grid.Ny, 1:f.grid.Nz)

The terminology is not perfect because of our convention for fields stored on faces (we store these fields asymmetrically: one boundary would be included in 'interior', but the other is not). However, this problem also plagues the current definition of data (and our notion of Field in general).

We can then also rename parentdata(f) to interiorparent(f), which returns a reference to the interior points of f.data.parent.

@ali-ramadhan
Copy link
Member

Hmmm, yeah I agree it would be good to be consistent with Base.parent.

I guess these aren't user-facing functions so they don't super memorable names. I like defining data to be consistent with parent and defining other functions for views, e.g. interiordata is nice.

I don't know if it makes sense to define data(f::AbstractArray) = f though.

@glwagner
Copy link
Member Author

data is very much a user facing function at the moment.

Note that parent(a::Array) = a.

@glwagner
Copy link
Member Author

glwagner commented Oct 15, 2019

Just to be clear, the point of doing this is because you want to use dispatch to write flexible array operations that are agnostic to whether the array is a 'raw' array (like an Array or CuArray), or some kind of wrapper like an OffsetArray. By writing parent(a), you ensure correct behavior on a in both cases; you don't need to write new high-level functions for wrappers versus arrays because dispatch is performed at the lower level, where it belongs. With data we can use the same logic --- this concept is deployed extensively in PR #463.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants