Skip to content

Default Stokes velocity subsolve to FGMRES (#147) #149

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.

Sign up for GitHub

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

Merged
lmoresi merged 1 commit into from
Apr 28, 2026
Merged

Default Stokes velocity subsolve to FGMRES (#147) #149

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions src/underworld3/cython/petsc_generic_snes_solvers.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3990,8 +3990,10 @@ class SNES_Stokes_SaddlePt(SolverBaseClass):
p_name = "pressure" # pressureField.clean_name
v_name = "velocity" # velocityField.clean_name

# Works / mostly quick
# Pressure subsolve — flexible GMRES + GASM. (FGMRES is right-
# preconditioned by construction; no need to set pc_side explicitly.)
self.petsc_options[f"fieldsplit_{p_name}_ksp_type"] = "fgmres"
self.petsc_options[f"fieldsplit_{p_name}_ksp_max_it"] = 200
self.petsc_options[f"fieldsplit_{p_name}_ksp_rtol"] = self._tolerance

Copilot AI Apr 28, 2026

Copy link

Choose a reason for hiding this comment

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

In init, fieldsplit_pressure_ksp_rtol is set to self._tolerance, but the tolerance property setter/docstring later defines pressure subsolve rtol as tolerance * 0.1. This creates inconsistent defaults depending on whether users touch tolerance. Consider deriving this from tolerance consistently (e.g., use the same multiplier here or call the tolerance setter after defaults are established).

Suggested change
self.petsc_options[f"fieldsplit_{p_name}_ksp_rtol"] = self._tolerance
self.petsc_options[f"fieldsplit_{p_name}_ksp_rtol"] = self._tolerance * 0.1

Copilot uses AI. Check for mistakes.
self.petsc_options[f"fieldsplit_{p_name}_pc_type"] = "gasm"
# self.petsc_options[f"fieldsplit_{p_name}_pc_gasm_type"] = "basic"
Expand All @@ -4003,8 +4005,20 @@ class SNES_Stokes_SaddlePt(SolverBaseClass):
# self.petsc_options[f"fieldsplit_{p_name}_pc_gamg_type"] = "agg"
# self.petsc_options[f"fieldsplit_{p_name}_pc_gamg_repartition"] = True

# Great set of options for gamg
self.petsc_options[f"fieldsplit_{v_name}_ksp_type"] = "cg"
# Velocity subsolve — flexible GMRES + GAMG.
#
# FGMRES (not CG/FCG) is the default to remain robust under non-stationary
# preconditioning and weakly-indefinite coarse operators. The mg_levels
# KSP is bounded but variable-iteration (mg_levels_ksp_converged_maxits)
# so the GAMG application is non-linear; CG/FCG's residual recurrence
# cannot accommodate this, and at scale (large parallel partitions, sharp
# internal sources, free-slip Nitsche) PETSc reports DIVERGED_PC_FAILED
# / indefinite matrix or GMRES residual-recursion mismatch. See issue
# #147 for the spherical-Kramer benchmark on Gadi that drove this change;
# gthyagi's validated FGMRES configuration completed at np=144,
# cellsize=1/32 where CG/FCG and standard GMRES both failed.
self.petsc_options[f"fieldsplit_{v_name}_ksp_type"] = "fgmres"
self.petsc_options[f"fieldsplit_{v_name}_ksp_max_it"] = 200
self.petsc_options[f"fieldsplit_{v_name}_ksp_rtol"] = self._tolerance * 0.1

Copilot AI Apr 28, 2026

Copy link

Choose a reason for hiding this comment

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

In init, velocity subsolve rtol is set to self._tolerance * 0.1, but the tolerance property setter/docstring later sets fieldsplit_velocity_ksp_rtol to tolerance * 0.033. This means the default tolerance behavior differs between “fresh solver” and “user set tolerance”. Recommend aligning these to a single rule (either update this default or make both paths reuse the same tolerance logic).

Suggested change
self.petsc_options[f"fieldsplit_{v_name}_ksp_rtol"] = self._tolerance * 0.1
self.petsc_options[f"fieldsplit_{v_name}_ksp_rtol"] = self._tolerance * 0.033

Copilot uses AI. Check for mistakes.
self.petsc_options[f"fieldsplit_{v_name}_pc_type"] = "gamg"
self.petsc_options[f"fieldsplit_{v_name}_pc_gamg_type"] = "agg"
Expand Down Expand Up @@ -4396,8 +4410,10 @@ class SNES_Stokes_SaddlePt(SolverBaseClass):
p_name = "pressure" # pressureField.clean_name
v_name = "velocity" # velocityField.clean_name

# Works / mostly quick
# Pressure subsolve — flexible GMRES + GASM. (FGMRES is right-
# preconditioned by construction; no need to set pc_side explicitly.)
self.petsc_options[f"fieldsplit_{p_name}_ksp_type"] = "fgmres"
self.petsc_options[f"fieldsplit_{p_name}_ksp_max_it"] = 200
self.petsc_options[f"fieldsplit_{p_name}_ksp_rtol"] = self._tolerance

Copilot AI Apr 28, 2026

Copy link

Choose a reason for hiding this comment

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

In strategy setter, pressure subsolve rtol is reset to self._tolerance, which overrides the tolerance property’s documented behavior (fieldsplit_pressure_ksp_rtol = tolerance * 0.1). If users do solver.tolerance = ... and later solver.strategy = "default", they’ll silently get a different inner-solve tolerance. Suggest keeping strategy from clobbering tolerance-derived options (or recompute them using the same multipliers/call the tolerance setter at the end).

Suggested change
self.petsc_options[f"fieldsplit_{p_name}_ksp_rtol"] = self._tolerance
self.petsc_options[f"fieldsplit_{p_name}_ksp_rtol"] = self._tolerance * 0.1

Copilot uses AI. Check for mistakes.
self.petsc_options[f"fieldsplit_{p_name}_pc_type"] = "gasm"
self.petsc_options[f"fieldsplit_{p_name}_pc_gasm_type"] = "basic"
Expand All @@ -4409,8 +4425,11 @@ class SNES_Stokes_SaddlePt(SolverBaseClass):
# self.petsc_options[f"fieldsplit_{p_name}_pc_gamg_type"] = "agg"
# self.petsc_options[f"fieldsplit_{p_name}_pc_gamg_repartition"] = True


self.petsc_options[f"fieldsplit_velocity_ksp_type"] = "cg"
# Velocity subsolve — see corresponding block in __init__ for the
# rationale (FGMRES default for robustness to non-stationary GAMG
# preconditioning and weakly-indefinite coarse operators; issue #147).
self.petsc_options[f"fieldsplit_velocity_ksp_type"] = "fgmres"
self.petsc_options[f"fieldsplit_velocity_ksp_max_it"] = 200
self.petsc_options[f"fieldsplit_velocity_pc_type"] = "gamg"
self.petsc_options[f"fieldsplit_velocity_pc_gamg_type"] = "agg"
self.petsc_options[f"fieldsplit_velocity_pc_gamg_repartition"] = True
Expand Down
Loading