Default Stokes velocity subsolve to FGMRES (#147) - #149
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the default PETSc configuration for the Stokes Schur/fieldsplit inner velocity solve to be more robust under non-stationary GAMG preconditioning, addressing convergence failures reported in #147.
Changes:
- Switch default
fieldsplit_velocity_ksp_typefromcgtofgmresin both theSNES_Stokes_SaddlePtconstructor and thestrategysetter defaults. - Add
ksp_max_it = 200bounds to both pressure and velocity fieldsplit sub-solves. - Expand in-code rationale/comments explaining why flexible Krylov is required in this nested-solver setup.
Comments suppressed due to low confidence (1)
src/underworld3/cython/petsc_generic_snes_solvers.pyx:4437
strategysetter setsfieldsplit_velocity_pc_mg_type = "kaskade", but__init__sets the velocity blockpc_mg_typeto "additive". This meanssolver.strategy = "default"does not actually reproduce constructor defaults (and contradicts the PR description about re-applying the same defaults). Consider consolidating the default fieldsplit configuration into a shared helper or make these values match.
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
self.petsc_options[f"fieldsplit_velocity_pc_mg_type"] = "kaskade"
self.petsc_options[f"fieldsplit_velocity_pc_gamg_agg_nsmooths"] = 2
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 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 |
There was a problem hiding this comment.
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).
| self.petsc_options[f"fieldsplit_{p_name}_ksp_rtol"] = self._tolerance | |
| self.petsc_options[f"fieldsplit_{p_name}_ksp_rtol"] = self._tolerance * 0.1 |
| # 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 |
There was a problem hiding this comment.
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).
| 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 |
| # 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 |
There was a problem hiding this comment.
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).
| self.petsc_options[f"fieldsplit_{p_name}_ksp_rtol"] = self._tolerance | |
| self.petsc_options[f"fieldsplit_{p_name}_ksp_rtol"] = self._tolerance * 0.1 |
Implements the FGMRES fieldsplit configuration that gthyagi diagnosed and validated at scale on Gadi (issue #147). Credit for finding the underlying CG/FCG / standard-GMRES failure mode in the spherical Kramer free-slip Nitsche benchmark, isolating the trigger conditions, and validating FGMRES as the working configuration goes to gthyagi; this commit only promotes that configuration to the UW3 default. Changes the default fieldsplit velocity KSP from CG to FGMRES across both the SNES_Stokes_SaddlePt constructor and the strategy setter (used when solver.strategy is reassigned). Adds an explicit ksp_max_it=200 safety bound to both pressure and velocity subsolves. Why --- The Schur fieldsplit preconditioner uses inner Krylov solvers (GAMG with mg_levels_ksp_max_it=3 and _converged_maxits=true) whose application is non-linear. CG/FCG and standard GMRES build their residual recurrence assuming a linear preconditioner, so the recurrence drifts from the true residual under non-stationary preconditioning. Combined with the weakly-indefinite coarse operators that GAMG produces on partition-stressed problems with sharp internal sources (Kramer case1's internal-boundary delta forcing), the velocity block trips PETSc's "DIVERGED_PC_FAILED / indefinite matrix" check or the GMRES residual-recursion mismatch. FGMRES handles non-stationary preconditioning by storing the preconditioned vectors explicitly rather than relying on the m-term recurrence; it is the right default for any solver that composes nested Krylov methods, independent of bilinear-form symmetry. gthyagi's at-scale validation (#147) showed FGMRES completing at np=144, cellsize=1/32 on Gadi where CG/FCG and standard GMRES both failed. Locally re-validated: pytest -m "level_1 and tier_a" on amr-dev passes 56/3/0. Compatibility ------------- - pc_side is intentionally not set: FGMRES is right-preconditioned by construction, and not setting pc_side keeps the defaults compatible with users who override the velocity KSP to CG/FCG (which require left preconditioning). - The 8 existing tests that explicitly set fieldsplit_velocity_ksp_type=fcg after solver construction continue to work — user overrides cleanly replace the default. - solver.strategy = "default" re-applies the new FGMRES defaults via the same code path. Underworld development team with AI support from Claude Code
9d16da2 to
5902ba3
Compare
Summary
Closes #147 (no UW3 doc note needed; the default just works).
Why
The Schur fieldsplit preconditioner uses inner Krylov solvers — GAMG with `mg_levels_ksp_max_it=3` and `_converged_maxits=true` — whose application is non-linear (variable iteration count per call). CG/FCG and standard GMRES build their residual recurrence assuming a linear preconditioner, so the recurrence drifts from the true residual under non-stationary preconditioning.
Combined with the weakly-indefinite coarse operators that GAMG produces on partition-stressed problems with sharp internal sources (Kramer case1's internal-boundary delta forcing), the velocity block trips PETSc's "DIVERGED_PC_FAILED / indefinite matrix" check or the GMRES residual-recursion mismatch.
FGMRES handles non-stationary preconditioning by storing the preconditioned vectors explicitly rather than relying on the m-term recurrence. It's the right default for any solver that composes nested Krylov methods, independent of whether the bilinear form is symmetric.
This is not about Nitsche introducing asymmetry — UW3's Nitsche default is symmetric (`theta=1`). The non-stationarity comes from GAMG.
Test plan
Validated by gthyagi (#147)
At-scale validation on Gadi using the proposed configuration:
CG/FCG and standard GMRES failed on this case; FGMRES completed cleanly.
Underworld development team with AI support from Claude Code