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

How to deal with cell-centered and face-centered fields in NetCDF output? #92

Closed
ali-ramadhan opened this issue Feb 28, 2019 · 12 comments
Closed
Labels
abstractions 🎨 Whatever that means numerics 🧮 So things don't blow up and boil the lobsters alive output 💾

Comments

@ali-ramadhan
Copy link
Member

All the arrays right now are of size Nx*Ny*Nz but technically there are Nx volumes in the x-direction and Nx+1 faces so the output should match this and make sense. The easiest thing to do with the doubly periodic configuration we have right now is to have row Nx+1 be a repeat of row 1 but writing out the fields at the very bottom (k=Nz+1) might require some extra computation?

@glwagner
Copy link
Member

If we are committed to not using ghost cells, then I think the solution will depend on how boundary conditions are implemented. Periodic boundaries are just one of many possible cases we will encounter.

@ali-ramadhan
Copy link
Member Author

Yeah the output writing might get complicated as it could depend on the grid and boundary conditions and #86.

Will also be more complicated when data is spread across multiple cores or GPUs.

@glwagner
Copy link
Member

This may be another reason why ghost cells are preferred. A “boundary condition” can then be thought of as a function that fills in missing values on the edges of a field. Currently a “boundary condition” is something that modifies the right hand side of an equation... ?

@ali-ramadhan
Copy link
Member Author

Should we open a new issue to discuss whether or not to use ghost cells? Might be a hot topic and is only a backend issue so it isn't related to #86 either I think.

@jm-c
Copy link
Collaborator

jm-c commented Feb 28, 2019 via email

@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Feb 28, 2019

@jm-c @glwagner I can do 3pm later today.

Scheduling meetings in GitHub issues is awesome!

@glwagner
Copy link
Member

3p works for me too.

@glwagner
Copy link
Member

glwagner commented Mar 1, 2019

Let it be recorded: we discussed ghost cells and decided not to use them.

The main reason for not using ghost cells is that it is not clear whether an algorithm based on ghost cells can be used for irregular boundaries, where a cell point within land may be shared between 2 or more adjacent fluid cells.

We will need to figure out how to include the correct values of FaceFields on the boundary in output, for arbitrary boundary conditions. Perhaps we can implement some kind of pseudo-ghost cell functionality that only affects the calculation of output. Something to stew over.

@ali-ramadhan ali-ramadhan added numerics 🧮 So things don't blow up and boil the lobsters alive abstractions 🎨 Whatever that means labels Mar 2, 2019
ali-ramadhan added a commit that referenced this issue Mar 20, 2019
This is temporary until we solve #92. At least this way you can read 
output and get back an array of the expected size (Nx*Ny*Nz).
@ali-ramadhan ali-ramadhan mentioned this issue Jun 11, 2019
@ali-ramadhan
Copy link
Member Author

Just noting that whether we should save boundary values (i.e. halo data) to disk is an open issue #92.

The user can choose how to save their data in general, so the point holds regardless of how the 'default' output writer is designed.

Originally posted by @glwagner in #100 (comment)

Good point. A default where halo data is omitted makes the most sense to me, but we can just have a save_boundary_data flag that saves the full offset array to disk in case anyone wants it.

@glwagner
Copy link
Member

Just wait until you start trying to calculate fluxes and gradients in post-processing... 😏

@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Jun 11, 2019

Ah, fair enough. I still think it would be confusing for most users to have the boundary values in the output by default but a save_boundary_data flag sounds like an important feature now (not just for NetCDF).

@ali-ramadhan
Copy link
Member Author

This has been recently resolved in PR #644.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abstractions 🎨 Whatever that means numerics 🧮 So things don't blow up and boil the lobsters alive output 💾
Projects
None yet
Development

No branches or pull requests

3 participants