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

Which topologies are actually supported? #1192

Closed
ali-ramadhan opened this issue Nov 23, 2020 · 10 comments
Closed

Which topologies are actually supported? #1192

ali-ramadhan opened this issue Nov 23, 2020 · 10 comments
Labels
question 💭 No such thing as a stupid question

Comments

@ali-ramadhan
Copy link
Member

Not sure if I should open another issue for this, but I was trying to set-up a 2D topology like (Bounded, Flat, Bounded) but couldn't. Apparently, however, (Flat, Bounded, Bounded) works.

Is there a list of available topologies to choose from? The topology page on the docs leads me to believe that they're all implemented but (unless I'm missing something) they're not. Is that list somewhere in the docs? I couldn't find it.

Cheers!

Originally posted by @tomchor in #1178 (comment)

@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Nov 23, 2020

Sorry about the confusion @tomchor! Opening a new issue might actually be a good idea since it's true we currently only support a subset of the 3^3 = 27 possible topologies (I went ahead and quoted your post in this new issue).

I actually might avoid using the Flat topology right now since until it's fully tested (see e.g. #1023).

You can substitute Bounded for Flat and your simulation should do the same thing except it will take up some extra memory and slow down your model by a tiny bit. The functional difference is that halo regions have non-zero size along Bounded dimensions (and derivatives are computed along Bounded dimensions which will evaluate to zero unless you have some weird boundary conditions).

Yeah currently the four supported topologies are:

  1. (Periodic, Periodic, Periodic)
  2. (Periodic, Periodic, Bounded)
  3. (Periodic, Bounded, Bounded)
  4. (Bounded, Bounded, Bounded) (unfortunately only works on the CPU, see Running on GPU with topology = (Bounded, Bounded, Bounded) #1007).

We could support all possible topology combinations. The only bottleneck is implementing pressure solver that work for all topologies (see #586). This is on my current list of things to do but is not super trivial (see JuliaGPU/CUDA.jl#119 and possibly related #1170) so it's taking some time... 😅

Hopefully I can refactor the pressure solver(s) to support all topologies soon but either way it might be good to be explicit about which topologies are actually supported.

@ali-ramadhan ali-ramadhan added the question 💭 No such thing as a stupid question label Nov 23, 2020
@tomchor
Copy link
Collaborator

tomchor commented Nov 23, 2020

Thanks!
I'm a bit confused though. I see some topologies in the examples that run successfully but that aren't in your list. For example the Langmuir example has this grid:

RegularCartesianGrid{Float64, Periodic, Periodic, Bounded}
                   domain: x ∈ [0.0, 128.0], y ∈ [0.0, 128.0], z ∈ [-96.0, 0.0]
                 topology: (Periodic, Periodic, Bounded)
  resolution (Nx, Ny, Nz): (32, 32, 48)
   halo size (Hx, Hy, Hz): (1, 1, 1)
grid spacing (Δx, Δy, Δz): (4.0, 4.0, 2.0)

Is it possible that you're listing the topologies in the z, y, x format instead of x, y, z?

@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Nov 23, 2020

Ah sorry you're right, good catch! I updated the original list with the actually supported topologies.

Although I guess a side note worth mentioning is that I think you can create a grid with any topology and it won't error until it tries to construct a pressure solver. Might be clearer and more user friendly to print a useful error/warning when constructing the grid.

@tomchor
Copy link
Collaborator

tomchor commented Nov 23, 2020

Yes, in my tests I only get an error when getting the pressure solver going. And I agree that an error when actually constructing the grid would be better! Although I think updating the docs is the priority. Would you like me to update it using your list in the previous comment?

@ali-ramadhan
Copy link
Member Author

If you have the time to add the list of supported topologies to the documentation that would be very much appreciated!

@tomchor
Copy link
Collaborator

tomchor commented Nov 23, 2020

Just included it here: #1193

This is my first time contributing so hopefully I followed the right protocol!

@ali-ramadhan
Copy link
Member Author

Looks great, thank you for the PR!

We've recently begun to follow the ColPrac guide (also PR #1044) which has some pretty nice guidelines on community package development.

@tomchor
Copy link
Collaborator

tomchor commented Nov 25, 2020

Should this be closed because of #1193 ?

@ali-ramadhan
Copy link
Member Author

ali-ramadhan commented Nov 25, 2020

Ah yes!

PS: If you add something like "Resolves #1192" to your PR then merging the PR will automatically the associated issue(s).

@tomchor
Copy link
Collaborator

tomchor commented Nov 25, 2020

Awesome! Thanks for the heads-up. I was wondering how to link those to their issues.

Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question 💭 No such thing as a stupid question
Projects
None yet
Development

No branches or pull requests

2 participants