Skip to content

Integrate Krylov.jl for pressure solve #4409

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

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

Conversation

xkykai
Copy link
Collaborator

@xkykai xkykai commented Apr 17, 2025

This PR aims to integrate KrylovSolver for the pressure solve which has been developed into Oceananigans for the nonhydrostatic model. I begun by developing a new solver called KrylovPoissonSolver, which is a Poisson solver which uses Krylov methods available in Krylov.jl.

There are a few considerations on the design that I think would be helpful to hear your thoughts on:

  1. Is there a place for both ConjugateGradientPoissonSolver and KrylovPoissonSolver? Should we keep both methods, or deprecate ConjugateGradientPoissonSolver?
  2. There are multiple choices of preconditioners we can choose from, both from Oceananigans and Krylov.jl. For now I have kept the preconditioners in the file src\Solvers\conjugate_gradient_poisson_solver.jl, while putting the new KrylovPoissonSolver in src\Solvers\krylov_poisson_solver.jl which is also compatible with the preconditioners we have. Should we reorganize the files so that we put preconditioners in a separate file? Then perhaps their relations would be clearer.
  3. Should we develop a unified interface to allow users to use preconditioners from Krylov.jl as well?
  4. I am also not good at writing summaries, so if people have ideas on how to write good Base.summary I am happy to hear your thoughts on what's important!

@glwagner @simone-silvestri @amontoison @tomchor

@glwagner glwagner changed the title Integrate Krylov.jl for pressure solve. Integrate Krylov.jl for pressure solve Apr 17, 2025
@amontoison
Copy link
Collaborator

amontoison commented Apr 17, 2025

I think a dedicated file for the preconditioners is a good idea.

The common API for preconditioners is ldiv!(y, P, x) or mul!(y, P⁻¹, x) (three arguments).
The current API in Oceananigans.jl is precondition! with a variable number of arguments.

It is because the update of the preconditioner and the application of it is fused in Oceananigans.jl.

Related documentation on preconditioners: https://jso.dev/Krylov.jl/dev/preconditioners/

@simone-silvestri
Copy link
Collaborator

I would just remove the ConjugateGradientSolver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants