Geometric-MG bundle owns its outer-KSP pairing: fgmres, applied to the live KSP - #515
Conversation
…o the live KSP (#514) Since #471 the bundle's default smoother is gmres/4. A Krylov smoother makes the preconditioner vary between applications, and a plain-gmres outer's recurrence assumes it fixed: measured on a 3-D adapt child (PR #510 CI, then locally), the KSP reported CONVERGED_RTOL at 3 iterations with the true residual stalled at 1.3e-6 - a hundred times the gate. The same failure was found and fixed locally twice before, on the Stokes velocity sub-KSP and on the free-surface solver, which is exactly the piecemeal drift #471 exists to end; this puts the pairing in the one owner. Three parts. ksp_type=fgmres joins the geometric bundle for BOTH smoother variants - richardson does not need flexibility, but the stale-key derivation requires every variant to own the same keys, and fgmres judges convergence on the true residual, which is never wrong. The three solver-default writers of ksp_type go through _push_managed_option, so the bundle may upgrade the DEFAULT but never a user's explicit choice (the ownership latch). And the top-level install path applies the resolved type to the LIVE KSP: the database write alone is provably inert there, because the KSP consumed its options long before injection - the new live test caught exactly that in its first run. The fieldsplit velocity sub-KSP needs no live application (its setFromOptions runs at the parent's PCSetUp, after the write) and the rotated path already writes fgmres unconditionally on its per-solve prefix. On hierarchies where one FMG application converges the cycle, the two pairings are indistinguishable (measured: 3.3e-8 either way on the 2-D fixture); the drift needs a hierarchy the smoother varies on, which is the 3-D one-level-per-doubling shape arriving with PR #510 - its test_0842 is the discriminating gate, and passes on the composed tree. Closes #514. Underworld development team with AI support from Claude Code
lmoresi
left a comment
There was a problem hiding this comment.
Adversarial review (authoring session).
Where we attacked it:
- The inert-fix trap — caught by our own test's first run. The obvious fix (put
ksp_typein the bundle) does nothing on the standard path: the top-level KSP consumes its options before injection, so the DB write never takes. The live-object application in_configure_pcmg(ksp=...)is the actual fix; the live test asserts onksp.getType(), not the database, for exactly this reason. - Ownership latch semantics. A user's
petsc_options["ksp_type"] = "cg"survives: the latch sees an unrecorded key and the bundle backs off; the live application then applies the user's value (a no-op setType). The three converted default writers are internal writers of a now-bundle key, which #471's own doctrine says must be recorded — this was a pre-existing violation of that rule, not a new pattern. - Negative control on the new live test. On the 2-D fixture the broken and fixed pairings are indistinguishable (3.3e-8 both, one FMG application — measured before setting the gate), so the error assert is a sanity bound only and says so; the discriminating gate is
test_0842on the 3-D subsampled hierarchy, which goes 1.0e-6 → 2.7e-10 with this fix and is part of PR #510's CI. - Blast radius.
fast/richardson variant: stationary smoother, fgmres merely judges on the true residual — sound, small extra memory. Fieldsplit velocity sub-KSP: bundle value equals current practice (fgmres, per the code's own comment). Rotated path: writes fgmres unconditionally already, untouched. GAMG fallback:ksp_typeis in its stale list, so falling back clears our key and returns the default — verified by the bundle unit test.
Recommend merge; #510's re-run against post-merge development is the integration proof.
Underworld development team with AI support from Claude Code
There was a problem hiding this comment.
Pull request overview
This PR fixes a PETSc Krylov-method pairing defect in Underworld3’s geometric multigrid (FMG) option bundle: when the MG smoother itself is Krylov-based, the preconditioner becomes non-stationary between applications, so a non-flexible outer GMRES can report convergence while the true residual stalls. The change makes the geometric-MG bundle responsible for enforcing a flexible outer KSP type, and ensures that resolved defaults are applied to the live KSP object (not just written into the options DB).
Changes:
- Add
ksp_type=fgmresto the geometric multigrid option bundle (for both smoother variants) and ensure GAMG treats that key as stale to avoid prefix leakage. - Route solver-default
ksp_typewrites through_push_managed_optionso bundle ownership can upgrade defaults without overriding user-owned settings. - Apply the resolved
ksp_typeto the live top-level KSP during custom-P FMG injection, and add tests that assert both bundle invariants and live-object behavior on an adapt-child scalar solve.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/test_1021_mg_option_bundle.py |
Adds regression tests for the geometric-MG “Krylov smoother ⇒ flexible outer” pairing and verifies the live KSP type on an adapt-child scalar solve. |
src/underworld3/utilities/multigrid_options.py |
Extends the geometric MG settings bundle to include ksp_type=fgmres so the bundle owns the outer/inner pairing rule. |
src/underworld3/utilities/custom_mg.py |
Threads the live top-level ksp into _configure_pcmg() and sets the resolved ksp_type directly on the live object after bundle application. |
src/underworld3/cython/petsc_generic_snes_solvers.pyx |
Switches solver-default ksp_type initialization to _push_managed_option() (and similarly for other bundle-owned defaults) to preserve bundle/user ownership semantics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…_Boundary removal (#503) Two reconciliations beyond the textual merge: - custom_mg.py: #515 puts ksp_type=fgmres in the geometric bundle and applies the resolved type to the live KSP from _configure_pcmg — the one-owner form of the same fix our _ensure_flexible_outer carried for the scalar custom-P route. #515's mechanism covers that route (the solver-default ksp_type=gmres is now a managed option the bundle may upgrade, and _install_transfers passes the live KSP), so our helper is removed outright; no residue is needed. The explicit-gamg opt-out gate in build_transfers survives unchanged. - Null_Boundary fixtures: #503 stopped manufacturing the every-vertex sentinel label. test_0844's vertex-blanket negative control now builds its own blanket label explicitly (the hazard is any vertex-blanket label, not that one spelling — and the fixture is now self-contained); stale comments in test_0848, test_0842 and reconnect.py's _interface_edges docstring are updated to match, with the vertex exclusion kept load-bearing for caller enums and old checkpoints. Underworld development team with AI support from Claude Code
…ndent pinning, explicit-GAMG opt-out, the #492 CI disarm (#530) * Make the pinned-band emptiness test collective, and create the label on every rank label_interface_band raised rank-locally when its own subdomain held no band cell. A rank the surface never enters legitimately has an empty local band — only a GLOBALLY empty band is a user error. With the rank-local raise, a corner-confined surface at np=4 left three ranks raising while the fourth entered the collective mover and hung to the 300 s timeout (measured 2026-08-06, review of PR #488). Now the pinned count is allreduced, the error is raised on every rank only when the band is globally empty, and the DMLabel is created on every rank — including band-less ones — so downstream consumers see a symmetric label. Underworld development team with AI support from Claude Code * Make the pinned band partition-independent: synchronise pinned vertices by coordinate The straddle test and the halo-ring growth both walk rank-local cells, and cells are partitioned disjointly, so a shared vertex whose cut (or ring) cell lives on the neighbour rank was pinned there but not on the owner. The mover then moved the owned copy and the neighbour's pinned copy followed through the SF: measured at np=4 (review of PR #488, 2026-08-06), two pinned leaves moved 4.2e-3 and 1.9e-3 while np=2/3 passed on partition luck. The pinned set is now synchronised across ranks by rounded coordinate after the core band and after each halo ring. The parallel test gains two guards for the band-less rank np=4 creates: getStratumIS on a strata-less DMLabel is a segfault (#291), and max() of a zero-size moved array raises rank-locally and desyncs the collectives that follow. Underworld development team with AI support from Claude Code * An explicit GAMG choice opts out of the custom-P multigrid pickup, without crashing build_transfers now stands down — returning (None, None), never bare None — when the solver carries an explicit preconditioner="gamg" or a latched _pc_user_override (the user wrote their own pc_type into petsc_options). Before this gate, the opportunistic mesh-owned pickup silently clobbered the explicit choice back to the custom-P PCMG at solve time: both arms of test_0842's fmg-vs-gamg comparison ran pc_type=mg, so a user could not opt out and the comparison was vacuous. Both call sites (auto_inject_custom_mg and the rotated free-slip _build_rotated_custom_Pl) guard the unpack so a "no hierarchy" answer degrades to the default preconditioner instead of a TypeError mid-solve — a bare return inside this gate is exactly what the #488 x #471 merge once shipped. Underworld development team with AI support from Claude Code * Disarm the #492 trap in test_0842 and make the fmg-vs-gamg comparison real Both MeshVariables are created before any solver runs: creating one after a solve rebuilds mesh.dm under the custom-MG coarse/fine links (issue #492), and that dangling reference is what segfaulted Linux CI downstream of this test. Each arm now asserts the PC type it actually ran (mg vs gamg), so the comparison can never silently become FMG-vs-itself again. The tolerance pair (ksp_rtol 1e-9, nodal bound 1e-7) is chosen to catch the once-shipped true-residual stall signature; the geometric bundle now owns its fgmres outer pairing (#514/#515), so the fmg arm's rtol is enforced in the true residual norm. Underworld development team with AI support from Claude Code * CI crashes print a Python stack: PYTHONFAULTHANDLER=1 in scripts/test.sh Underworld development team with AI support from Claude Code
…le-field FMG unlock, and multiplier_schur_pc verified (#478 #483 #484 #486) (#534) * Make every preconditioner fallback readable: solver.pc_fallbacks records what degraded, where, and why (#484) Ten of the twelve places a solver silently degrades its preconditioner (the single-field FMG gate, missing hierarchy, transfer-build failures, guard skips, forced Galerkin, the rotated-path substitutions) left no queryable state — a warning at best, nothing at worst — so neither a user nor a test could ask "did I get what I asked for?". Every site now writes through one recorder (_record_pc_fallback) into a public read-only property, solver.pc_fallbacks, with a fixed reason vocabulary: unavailable / declined / build_failed / check_skipped / forced. The record is cleared whenever the preconditioner options re-resolve (the same staleness rule as _pc_resolved), and solve-time sites (custom_mg, rotated_bc) re-record each solve. Warnings are unchanged where they exist; the silent "auto" declines gain a record but deliberately NO new warning. rotated_bc mirrors its two degraded arms into the same record; its own ctx keys stay authoritative. Behaviourally inert: no option value, route, or default changes. tests/test_1022_pc_fallback_observability.py: every probe proven to fire and proven silent on a clean solve (empty record is the global negative control). Underworld development team with AI support from Claude Code * Honour user-set snes_rtol/ksp_atol on the saddle-point solvers, and derive tolerance margins through one documented mechanism (#483) The Stokes solve re-pushed snes_rtol and ksp_atol before every solve (_reassert_outer_tolerances), silently discarding a value the user set explicitly — the worst reachability middle ground: documented as settable, actually owned. The re-push now runs through the same recorded-ownership latch that made snes_max_it reachable (ruling D18), generalised to a dict (_resolve_owned_option/_push_owned_option, with _resolve_snes_max_it kept as a named delegate): the framework keeps asserting the tolerance-derived values until the user sets a key, after which their value is honoured across solves. A user who never touches the keys sees byte-identical behaviour (proved by the no-override arms). Second half: Stokes and Stokes_Constrained derived different option keys from `tolerance` through two unrelated hand-rolled setters. Both now apply a per-class table (_TOLERANCE_DERIVED_KEYS, the base table keeping its historical _INNER_RTOL_MARGIN name as an alias) through one base method (_derive_tolerance_margins), at SET time only. The difference in table contents is deliberate and now documented: the base class derives the inner fieldsplit margins; Constrained derives the outer ksp_rtol and the Eisenstat-Walker pins, because EW pinning owns its outer accuracy. Constrained.__init__'s duplicate EW writes route through the same helper. Both `tolerance` docstrings now state the ownership table (OWNED vs DERIVED-at-set-time keys, and the EW caveat on ksp_rtol). tests/test_1023_saddle_tolerance_ownership.py reads the LIVE PETSc objects after two solves (the second solve is the one that used to clobber), with no-override negative controls on both classes. Stakeholder tests that write these keys (0820, 1013, 1014x2) pass. Underworld development team with AI support from Claude Code * Honour explicit geometric multigrid on single-field solvers via injection-free custom-P transfers (#478) preconditioner="fmg" on a scalar/vector solver was declined to GAMG with a warning, because the NATIVE geometric-FMG path needs DMCreateInjection, which PETSc cannot reliably build for a single-field discretisation on a refined DMPlex (err62, #276). That gate locked geometric MG out of every scalar/vector solver even though the robust custom-P route (own prolongations + Galerkin RAP, no injection anywhere) was already in utilities.custom_mg. The explicit request is now honoured: the gate flags the reroute (_pc_single_field_geo_requested) and custom_mg.build_transfers grows a THIRD hierarchy source — requested-native, wrapping the mesh's own dm_hierarchy tail in _DMLevelView adapters — resolved after solver-set and mesh-owned so build_transfers stays the single "which hierarchy" owner (#471, composing with #530's explicit-gamg opt-out, which is untouched). Installation rides the existing auto_inject_custom_mg solve hooks and the shared multigrid_options bundle (#468/#515), so the options DB deliberately keeps GAMG as the safe degrade base until the live PC is configured at first solve. Build failure degrades to that base through the recorded barycentric -> RBF -> default ladder (#484). Contract line, stated in both docstrings: preconditioner="fmg" is a PREFERENCE (degrades readably); set_custom_fmg is a DEMAND (raises). "auto" is deliberately unchanged — flipping the single-field default is its own validation campaign per #478 — it keeps GAMG and records the decline as the migration probe. Found en route: a gmsh-imported BASE hierarchy level carries section-only coordinates (PetscContainer, no PetscFE), and DMCreateInterpolation from such a source silently returns a ZERO matrix; _DMLevelView therefore works on a clone and installs a P1 Lagrange coordinate FE when missing. tests/test_1020 becomes the unlock test (live PC "mg" over every hierarchy level on the exact #276 err62 geometries; degrade arm proven by monkeypatched builders; auto-unchanged arm; Stokes velocity block untouched; DB-vs-live honesty arm) and tests/parallel/ptest_1020_fmg_single_field_parallel.py proves route parity at np=2. Underworld development team with AI support from Claude Code * multiplier_schur_pc: prove where it is live, instrument where it is inert, correct the drifted docstrings (#486) The issue measured four identical convergence rows across two decades of viscosity contrast and asked "verify or remove". Tracing PETSc's fieldsplit.c settles it without ambiguity: the flag swaps only the Pmat (h,h) block, and under Stokes_Constrained's own defaults (selfp + diag_use_amat) selfp assembles the Schur preconditioner from AMAT sub-blocks — the swapped block is provably never read, which is exactly what the issue measured. The flag IS read under pc_fieldsplit_schur_precondition='a11' (Sp = the grouped [p,h] Pmat block) and under a monolithic direct factorisation of the Pmat. Verdict: keep + instrument, not remove. Instrumentation: at the hh_pc selection in _setup_solver, an explicit opt-in that cannot reach the PC (not a11, diag_use_amat set, not a direct solve) records reason='declined' in pc_fallbacks AND warns — an explicit opt-in silently doing nothing is exactly the #477 class. Docstrings: multiplier_schur_pc now states its two live regimes and the inert one; the Constrained saddle_preconditioner claim that selfp uses "the 1/viscosity mass from constitutive_model.K" was drifted (selfp reads the Amat; the 1/mu mass participates only under a11) — corrected, with a TODO(BUG) on whether selfp should see Pmat blocks at all (a design question, deliberately not changed here). tests/test_1024_multiplier_schur_pc.py: matrix-level oracle (assembled Schur pre differs flag-on/off under a11 — immune to "both converge in 4 iterations"), the inertness codified as an exact-equality control pinning the PETSc semantics, regime-aware probe silence, no-flag silence, and the setter re-registration contract. Underworld development team with AI support from Claude Code * Review response: a resolution-cached hierarchy yields to a new explicit choice; the inertness probe reads the flag's value Two defects from the #534 adversarial review, both violations of the bundle's own observability contract. (1) The explicit-fmg install cached its hierarchy in solver._custom_mg, which auto_inject re-installs unconditionally — so a later preconditioner="gamg" was unreachable with an EMPTY pc_fallbacks record. The cache is now marked auto_cached and the preconditioner setter drops it; a set_custom_fmg registration is a demand and is kept. Regression test with a verified negative control (fails with the marker disabled). (2) The multiplier_schur_pc inertness probe read hasName("pc_fieldsplit_diag_use_amat") — the key's presence — so diag_use_amat=false (the flag LIVE, Schur pre measured differing by rel-Frobenius 0.30) still warned "no effect" and recorded a decline. It now reads the bool value; regression test pins the flag=false silence. Underworld development team with AI support from Claude Code
adapt-on-top-faults: the rotated free-slip path now picks up an adapt child's mesh-owned MG tail automatically (custom_mg.build_transfers, the #467 fix) — the "FUNDAMENTAL, not a quick fix" gotcha row was describing a bug that is gone. Point the band-sizing section at the interface-alignment primitives that now exist (place_sheet / place_thin_volume / remove_embedded, #517-#526). nonlinear-solver: three stale claims corrected — rotated free-slip is no longer the exception to the automatic tail pickup (#467); the adapt tail is one MG level per DOUBLING of h, not per refinement generation (mg_coarsening_ratio=2.0, #515); refinement=0 still yields a hierarchy that starts at the base, so "no coarse grid" overstated it. Three capabilities that landed since the branch: preconditioner="gamg" is respected on adapt children (#530), single-field FMG (#478/#534), and solver.pc_fallbacks as the observability hook (#534). The supersession note no longer names the removed in-SNES ramp API. Underworld development team with AI support from Claude Code
…current; plasticity-solvers rewritten (#454) (#489) * skills: parallel adapt engines, band pinning, and FMG on adapt children adapt-on-top-faults - engines section: nvb vs edge_split, both parallel in 2-D and 3-D and bit-confluent; edge_split has no conforming closure so refinement cannot escape the marked region, and marks on the DIAMETER (the volume proxy reported the target met while the mesh was 3.2x coarser across the feature). - repair=True: gates on reducing the largest angle, NOT on Delaunay. Delaunay maximises the minimum angle while P1 depends on the maximum, and flipping a gmsh mesh toward Delaunay raised the 99th-percentile max angle. Worth it on a poor base (156 -> 115 degrees, slivers 3.84% -> 0.00%), marginal on a clean one, and it gives up bit-confluence, so it is opt-in. - relax on a mesh refined onto an interface makes things WORSE (+77% leak); pin_bands is the fix. - new section on sizing the band and representing the fault margin: the -2 Cov(eta, edot) leak metric, why a within-cell marking rule loses to the plain distance size field, why the optimal band width depends on which objective you pick, and what a step-edged margin buys and costs. - gotchas: Mesh(dm) takes the DM over (bare SIGSEGV if you keep using the old handle); Mesh(dm) without boundaries= loses the boundary enum; evaluate() "Total components 8 != 6" on a variable-heavy mesh. adaptive-meshing - PIN THE INTERFACE section for relax(pin_bands=...), including the signed-vs-unsigned distance rule and the pinned_labels merge trap. - cross-reference to nonlinear-solver for the FMG setup. nonlinear-solver - new section: FMG on an adapt-on-top child. The child carries its own graded custom-P tail and solvers pick it up automatically; the base must have refinement>=1; a base-only tail triples the V-cycle count; V-cycle counts are insensitive to element quality (a pass, not a failed measurement) so use GAMG as the quality probe; relax can trip #424 into the dense RBF fallback; repair invalidates the any-degree transfer but not the vertex prolongation. - cross-references to adapt-on-top-faults and adaptive-meshing. Underworld development team with AI support from Claude Code * skills: bring the two adapt/solver skills current with development adapt-on-top-faults: the rotated free-slip path now picks up an adapt child's mesh-owned MG tail automatically (custom_mg.build_transfers, the #467 fix) — the "FUNDAMENTAL, not a quick fix" gotcha row was describing a bug that is gone. Point the band-sizing section at the interface-alignment primitives that now exist (place_sheet / place_thin_volume / remove_embedded, #517-#526). nonlinear-solver: three stale claims corrected — rotated free-slip is no longer the exception to the automatic tail pickup (#467); the adapt tail is one MG level per DOUBLING of h, not per refinement generation (mg_coarsening_ratio=2.0, #515); refinement=0 still yields a hierarchy that starts at the base, so "no coarse grid" overstated it. Three capabilities that landed since the branch: preconditioner="gamg" is respected on adapt children (#530), single-field FMG (#478/#534), and solver.pc_fallbacks as the observability hook (#534). The supersession note no longer names the removed in-SNES ramp API. Underworld development team with AI support from Claude Code * skills: rewrite plasticity-solvers around what the yield campaigns measured Fixes #454. The skill taught the retired yield-homotopy doctrine in ~8 places, headlined by an enable method that no longer exists in src/ and an in-SNES delta-ramp that is separately proven to diverge. The doctrine rested on a unit-scaling error: re-measured on the corrected problem, the delta-march never succeeded where a direct hard-Min solve failed, and the ruling was to regularise the problem, not the solver. Rewritten around the evidence: Newton with the automatic Picard entry (Picard is an entry requirement, not an accelerator), rescue on failure OR stagnation, grid sequencing as the validated warm start, the per-model tangent table, and the #475 yield_mode / yield_smoother / yield_anchor substrate presented as a modelling choice with the multi-solve-only discipline for any delta march. yield_continuation is described honestly per open issue #473 (the cold-start guarantee does not hold on a Piecewise yield stress; the step control is effectively one-shot). Floors updated to the post-#475 semantics (viscosity_min_rounding). Kept: the Newton-confirmation check, the VEP-indefinite ruling, the SNESFAS ruling, the Picard-folklore footnote. Underworld development team with AI support from Claude Code * The nonlinear-solver recipe demotes the delta-march to a rescue The recipe's step 2 still presented solve(homotopy=True) as the default entry point ("one call - automatic"), which is the retracted doctrine in its post-API form: the evidence that recommended a delta-march first rested on a unit-scaling error, and the driver's cold-start guarantee is broken (issue #473). The step now escalates honestly - grid sequencing first, the delta-continuation as rescue of last resort with the #473 caveat stated - and the Layer-2 status note carries the same demotion. The delta-discipline itself (constant per solve, never in-SNES) is unchanged; plasticity-solvers holds the ruling and evidence. Underworld development team with AI support from Claude Code
Fixes #514. First car of the landing train recorded in the planning-hub liaison ledger: this → #510 → #511.
The defect
Since #471 the geometric-MG bundle's default smoother is
gmres/4. A Krylov smoother makes the preconditioner vary between applications, and a plain-gmres outer's recurrence assumes it fixed. Measured on a 3-D adapt child (PR #510's CI, reproduced locally): the KSP reported CONVERGED_RTOL at 3 iterations while the true residual stalled at 1.3e-6 — one hundred times the gate. The same failure was previously found and fixed locally, twice: the Stokes velocity sub-KSP (the bundle's own comment records the reason) and the free-surface solver ("false-converges … TRUE residual blew up",systems/solvers.py). Piecemeal fixes of one pairing rule are the drift #471 exists to end.The fix, three parts
ksp_type = fgmresjoins the geometric bundle, both smoother variants — richardson does not need flexibility, but every variant must own the same key set (the stale-key derivation requires it), and fgmres judges convergence on the true residual, which is never wrong.ksp_typenow go through_push_managed_option, so the bundle may upgrade the default but never a user's explicit choice (petsc_options["ksp_type"] = ...still wins, per the One owner for the geometric-MG option bundle; rotated free-slip picks up a mesh-owned hierarchy (#468, #467) #471 ownership latch).setFromOptionsruns at the parent'sPCSetUp, after the write); the rotated path already writes fgmres unconditionally on its per-solve prefix.Verification
pc=mg+ksp=fgmreson the live objects.test_1021(20),test_0842(4), rotated paralleltest_1064at np=2 (9), Poisson/Stokes ND smoke (9) — all pass.test_0842passes at its 1e-8 gate (was 1.0e-6 broken; 2.7e-10 fixed), 40 tests green across 0842/1021/0753.test_0842is the discriminating gate.Underworld development team with AI support from Claude Code