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

Convention for definition of Field "halo points" is potentially confusing #455

Closed
glwagner opened this issue Oct 10, 2019 · 3 comments
Closed
Labels
cleanup 🧹 Paying off technical debt

Comments

@glwagner
Copy link
Member

Currently we use a convention for fields wherein the "halo points" of a field stored on faces actually includes locations that are on the boundary of our domain.

Is this strange? What I am saying, in other words, is that for the field u = Field{Face, Cell, Cell}(arch, grid), the point

u[grid.Nx+1, 1, 1]

is defined as a "halo point", even though it is located on the right x boundary (it is inside the physical domain, not outside it).

I think this is potentially odd and we should consider adopting the convention that Face fields include their boundary points by default. This would not change the algorithm; it only changes the user API.

The reason why I recommend considering / discussing this change rather than immediately adopting it is because there are some downsides. One is a slight increase in memory allocation that is not needed. This is minor for large 3D problems but could be annoying for small 1D or 2D problems with "flat" dimensions.

This last problem can potentially be solved by invoking special behavior for the "flat" dimensions of a grid (see #330).

@ali-ramadhan
Copy link
Member

From a user's perspective, this is most important when saving output as I guess you expect all N+1 faces to be present in the output. So I agree that the "interior points" should include that first halo point. If we just have interior_points(::Field) functions then we don't even need to increase memory usage.

Or are you suggesting we switch grids to 65x64x64 for u, 64x65x64 for v, and 64x64x65 for w like DIABLO does.

@glwagner
Copy link
Member Author

I am saying that it is confusing to call the point N+1 a halo point. It is not a halo point. It is on the boundary.

@ali-ramadhan ali-ramadhan added the cleanup 🧹 Paying off technical debt label Oct 29, 2019
@glwagner
Copy link
Member Author

This is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup 🧹 Paying off technical debt
Projects
None yet
Development

No branches or pull requests

2 participants