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

Finite volume operators #115

Closed
ali-ramadhan opened this issue Mar 7, 2019 · 4 comments · Fixed by #283
Closed

Finite volume operators #115

ali-ramadhan opened this issue Mar 7, 2019 · 4 comments · Fixed by #283
Assignees
Labels
abstractions 🎨 Whatever that means feature 🌟 Something new and shiny performance 🏍️ So we can get the wrong answer even faster

Comments

@ali-ramadhan
Copy link
Member

Right now the element-wise operators are all written for the constant Δx, Δy, Δz case. As we transition to supporting variable grid spacings, the operators should be written as finite volume operators that take into account the variable areas and volumes.

We should be able to figure out a way to write the operators such that they work efficiently for all Cartesian grids.

Would be good to do this before #47 is implemented. This may require #59 to be resolved first.

@ali-ramadhan ali-ramadhan added performance 🏍️ So we can get the wrong answer even faster abstractions 🎨 Whatever that means labels Mar 7, 2019
@ali-ramadhan ali-ramadhan added this to the v0.5 milestone Mar 7, 2019
@glwagner
Copy link
Member

glwagner commented Mar 8, 2019

It does seem like solving #59 will make dispatch a lot easier.

What needs to be done to solve #59? Let me know if I can help.

@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Mar 8, 2019

I was thinking of doing some prototyping and benchmarking in a sandbox by building off the example in my PR vchuravy/GPUifyLoops.jl#18.

The PR contains an example that can be extended to rely on a Grid struct, multiple FaceFields and CellField. So I'll prototype grids and fields that are isbitstype (you already helped by doing this for a grid in #59 (comment)) and test to see if they work on the GPU with GPUifyLoops.jl. If they do work and performance isn't degraded then I'll rewrite the operators to use grid and field structs.

You probably know how to do this better than me, but might be good if I rewrite the operators as they's still undocumented and do some slightly convoluted stuff to avoid having to store intermediate calculations.

Right now I'm focusing on system tests and benchmarks but once @christophernhill @jm-c and I get closer to implementing the variable Δz grid #47 I will work on this.

@glwagner
Copy link
Member

glwagner commented Mar 8, 2019

Okay, sounds good!

Let's not focus too much on optimization prematurely, though if I understand you I think you are really talking about the obvious things, like avoiding memory allocation and transfers to/from the CPU to GPU.

@ali-ramadhan ali-ramadhan pinned this issue Mar 21, 2019
@ali-ramadhan ali-ramadhan modified the milestones: v0.5, v0.6 Apr 3, 2019
@ali-ramadhan ali-ramadhan self-assigned this Apr 3, 2019
@ali-ramadhan ali-ramadhan unpinned this issue Apr 5, 2019
@ali-ramadhan ali-ramadhan modified the milestones: v0.6, v0.7 Apr 11, 2019
@ali-ramadhan ali-ramadhan pinned this issue Jun 4, 2019
@ali-ramadhan ali-ramadhan changed the title True finite volume operators that work on all Cartesian grids finite volume operators Jun 11, 2019
@ali-ramadhan ali-ramadhan changed the title finite volume operators Finite volume operators Jun 11, 2019
@ali-ramadhan
Copy link
Member Author

The title suggested that the finite volume operators in PR #283 are for Cartesian grids but they should actually work on all grids, even spherical or cubed sphere as long as the grid spacings are stored correctly and the halo communication is done right.

@ali-ramadhan ali-ramadhan added the feature 🌟 Something new and shiny label Jun 11, 2019
@ali-ramadhan ali-ramadhan unpinned this issue Oct 18, 2019
@ali-ramadhan ali-ramadhan removed this from the Finite volume operators milestone Oct 21, 2019
@ali-ramadhan ali-ramadhan pinned this issue Oct 22, 2019
@ali-ramadhan ali-ramadhan unpinned this issue Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abstractions 🎨 Whatever that means feature 🌟 Something new and shiny performance 🏍️ So we can get the wrong answer even faster
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants