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

Add ImmersedPressureSolver with FFTBasedPoissonSolver as preconditioner for complex domains #3188

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

xkykai
Copy link
Collaborator

@xkykai xkykai commented Jul 17, 2023

Using the FFTBasedPoissonSolver in complex domains on the ImmersedBoundaryGrid leads to resulting solution that either

  1. Does not conserve tracers in the flow; or
  2. Leads to divergent flow along the immersed boundary.

On the other hand, using the PreconditionedConjugateGradientSolver on complex domains leads to exact solution at a much slower speed on the GPU. This PR implements an ImmersedPressureSolver that uses the FFTBasedPoissonSolver as a preconditioner, then iterates PreconditionedConjugateGradientSolver to arrive at the correct solution. This approach is much faster than using a vanilla PreconditionedConjugateGradientSolver as the number of iterations scale much more slowly with grid size on the GPU.

This PR will include the ImmersedPressureSolver, a few validation examples for benchmarking purposes and a number of example use cases where this solver would be useful.

@glwagner @simone-silvestri

@navidcy navidcy added numerics 🧮 So things don't blow up and boil the lobsters alive immersed boundaries ⛰️ Less Ocean, more anigans labels Jul 18, 2023
@navidcy navidcy changed the title adding ImmersedPressureSolver with FFTBasedPoissonSolver as preconditioner for complex domains Add ImmersedPressureSolver with FFTBasedPoissonSolver as preconditioner for complex domains Jul 18, 2023
return pressure
end

struct MITgcmPreconditioner end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we name this something bit more descriptive and less referential to MITgcm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like SymmetricLocalPreconditioner? I'm not entirely sure, could use some ideas too!

Copy link
Collaborator

@navidcy navidcy Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I that sounds much better because at least it tries to describe something and not just refer to some other model.

I'm not sure what this preconditioner is doing so that's why I'm not in position to propose something. I don't even know what local refers to. But definitely a SomethingThatDescribesMePreconditioner is better than SimilarToSomeOtherModelOrSomebodyElsesPreconditioner. That's what I wanted to convey with my comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this preconditioner is some kind of perturbation about a diagonal preconditioner, is that right? DiagonallyDominantPreconditioner or something. In fact shoudln't we also delete the wrong version that's already present @simone-silvestri ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, it should be the same one but corrected to be 3D and with the correct denominator (we should correct also the 2D version of it)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so maybe DiagonallyDominantThreeDimensionalPreconditioner and also TwoDimensional.

@tomchor
Copy link
Collaborator

tomchor commented Apr 2, 2024

I'm not sure what the state of this PR is, but last I checked it was working, but was about 5 times slower than the FFT-based solver, no?

Coming from the discussion in #3529, I'd argue that it'd be useful to include this solver in the main code, even if it's a bit slow for now. Then it'd be easy to check the influence of near-immersed-boundary divergence when investigating results such as #3529.

Furthermore, I can see situations where I'd be willing to spend 5 times longer running simulations in order to be more sure of the results. Oceananigans can be so fast on GPUs that a 5x slower code might still be as good as a more traditional CPU code running the same problem 😂

What do you guys think?

@glwagner
Copy link
Member

glwagner commented Apr 2, 2024

I'm not sure what the state of this PR is, but last I checked it was working, but was about 5 times slower than the FFT-based solver, no?

Coming from the discussion in #3529, I'd argue that it'd be useful to include this solver in the main code, even if it's a bit slow for now. Then it'd be easy to check the influence of near-immersed-boundary divergence when investigating results such as #3529.

Furthermore, I can see situations where I'd be willing to spend 5 times longer running simulations in order to be more sure of the results. Oceananigans is can be so fast on GPUs that a 5x slower code might still be as good as a more traditional CPU code running the same problem 😂

What do you guys think?

There's no reason to wait on merging this as far as I'm concerned. @xkykai and @simone-silvestri can perhaps comment on their strategy / plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
immersed boundaries ⛰️ Less Ocean, more anigans numerics 🧮 So things don't blow up and boil the lobsters alive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants