Oceananigans.jl Bug in `DiagonallyDominantPreconditioner`

The precondition! defined for DiagonallyDominantPreconditioner takes the arguments:

P_r, ::DiagonallyDominantInversePreconditioner, r, ∫ᶻ_Axᶠᶜᶜ, ∫ᶻ_Ayᶜᶠᶜ, ...

https://github.com/CliMA/Oceananigans.jl/blob/2edd2b1fa321725ad4e7891d3442ca9dd0657d98/src/Models/HydrostaticFreeSurfaceModels/pcg_implicit_free_surface_solver.jl#L243

however, the fourth argument to precondition is supposed to be the free surface displacement (ie, the solution to the implicit step equation) as in

https://github.com/CliMA/Oceananigans.jl/blob/2edd2b1fa321725ad4e7891d3442ca9dd0657d98/src/Solvers/preconditioned_conjugate_gradient_solver.jl#L191

as I recall this preconditioner has never worked properly (ie it slows down convergence rather than speeding it up). Is this the reason?

cc @simone-silvestri

Jul 15 '22 21:07 glwagner

I see, I wonder why we need a free surface there since z = P * r, but indeed the signature is wrong!

Jul 15 '22 21:07 simone-silvestri

It seems we have designed the solver interface (ie, the function signature to precondition!) to include the guess / solution at the current iteration. This does seem useful for some preconditioners, right? Eg, if the preconditioner is an asymptotic solution that depends on the current guess, this may be useful. I think.

It's not necessary for the specific case of the diagnoally dominant preconditioner though -- I agree with that.

Jul 15 '22 21:07 glwagner