Solver configuration audit: reachability and silent fallback (report) - #485
Merged
Conversation
…llback Diagnostic audit against `development` at 0437958. No code changes. Five defects of the same shape were found independently over the last few weeks (#276/#478, #467, #468, #477, #425). Every one degrades to something that SUCCEEDS, so none presented as a failure — they presented later as "this is a bit slow", or as a sweep returning identical rows, which reads as "this variable does not matter" rather than "this variable was never set". This audit looks for the rest, in two directions: reachability (the user writes a setting and something else runs) and silent substitution (the code chooses otherwise and nothing observable says so). Findings are ranked by how INVISIBLE the failure is, not by the size of the substitution. Method: per family, set distinctive values, solve TWICE, pc.setUp(), read the LIVE PETSc objects, with a defaults arm as the control so a "reachable" verdict cannot be an accident of the override matching the default. Headline findings: - F-1 (new) The MG bundle silently overwrites user settings, and asking more explicitly makes it worse. The override latch keys on `pc_type` ALONE, so nine of the bundle's ten keys are discarded unless the user also sets the tenth — and `preconditioner="fmg"` bypasses the latch entirely. Measured all three ways. Belongs in #471, which makes the bundle a single owner and would otherwise entrench it. - F-3 (new) Stokes and Stokes_Constrained disagree about which options are reachable, in OPPOSITE directions: `ksp_rtol` is settable on one and not the other, the fieldsplit rtols the reverse. Scalar and vector families are clean, so the whole class is localised to the saddle-point path. - F-5 (new) The #477 fix is committed but UNPUSHED — three commits on feature/regime-diagram not on origin, so PR #475 does not show them and an auditor rediscovers a solved problem. It covers snes_max_it and the two fieldsplit rtols; it does NOT cover the MG bundle keys, which are a build-time rather than a solve-time clobber. Also records the #471/#475 collision on the stale-key list: #471's derivation subsumes #475's patch, so whichever lands second takes the derivation rather than merging the lists. - F-6 (new) Of twelve substitution points in the config surface, only two leave readable state — `velocity_pc` and `schur_pre`, both added in #465, and the only reason #467 was findable. A warning is not observability: warnings are filtered and deduplicated, state can be asserted on in a test. There is no public way to ask a solver which preconditioner it actually got, or why. - F-4 / F-7 / F-8 `snes_rtol` is owned but looks settable (decide-and-document); the barycentric->RBF retry silently switches to a DENSE transfer; two silent swallows can skip correctness guards. Each finding is routed to the PR it belongs in rather than to a new one. Harness and raw results: ~/+Simulations/solver_reachability_audit/ with a README. Underworld development team with AI support from Claude Code
…483/#484 F-1 is fixed in #471 (5a390ef). F-3/F-4 filed as #483, F-6/F-7/F-8 as #484, F-5 raised on #475. Adds the two lessons from implementing F-1: the ownership record must be keyed by the globally-qualified option name (an unqualified record made the bundle silently stop applying on the custom-P route), and the defaults arm is what caught that — the second ownership regression in two sessions caught by the arm that was supposed to be unchanged. Underworld development team with AI support from Claude Code
🔍 Architectural Review ChecklistThank you for submitting an architectural review! Reviewers should validate: Design & Architecture
Implementation
Testing & Validation
Documentation
Review Process: See CODE-REVIEW-PROCESS.md Approval: This PR merge = Review formally approved |
|
✅ Test Suite: success |
Phases 1 and 2 checked option KEYS and fallback BRANCHES and missed solver.strategy, which validated three values and configured one. Phase 3 covers that third axis: for each solve-configuring property, set two distinct values and check whether round-trip, options, J/Pmat, |u|, |p| or iteration counts move. The transferable finding is that the instrument was wrong three times first, each time confidently: 1. options compared across different solver prefixes, so every pair 'differed' and everything read live — including the negative control, which is what caught it; 2. options read BEFORE the solve, when most configuration is written at _build, which reported strategy and preconditioner inert when they are not; 3. blind observables — a pressure nullspace need not move the velocity, a preconditioner flag need not move the answer. Three of four 'still inert' verdicts were artefacts of measuring |u| alone. Once sound it independently rediscovered two known defects on development (strategy, and preconditioner on single-field solvers = #478), which is what makes its negative results worth anything. Cleared: tolerance, penalty, saddle_preconditioner, preconditioner, consistent_jacobian (live only on a NONLINEAR problem, 10 -> 5 nonlinear its), petsc_use_nullspace, petsc_use_pressure_nullspace (live on |p|, invisible in |u|), constant_nullspace, petsc_use_constant_nullspace, smoothing, smoothing_length. One unresolved: multiplier_schur_pc is read but moved no observable at constant viscosity or at eta contrast 1e4/1e6 — including an identically-failing solve. Recorded as verify-or-remove rather than as a dead flag. Also records a property that gets it right: constant_nullspace refuses an invalid combination naming the offending boundaries and the remedy. Underworld development team with AI support from Claude Code
|
✅ Test Suite: success |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Diagnostic report, no code changes. Baseline
development@04379583.docs/reviews/2026-07/solver-configuration-reachability-audit.md, with the harness and raw results under~/+Simulations/solver_reachability_audit/.Why
Five defects of the same shape were found independently and by accident over a few weeks — #276/#478, #467, #468, #477, #425. Every one degrades to something that succeeds: GAMG converges, richardson converges, the solve returns a correct answer. So none presented as a failure. They presented later as "this is a bit slow", or as a sweep returning identical rows, which reads as "this variable does not matter" rather than "this variable was never set". That cost a twelve-cell sweep on #477 and a wrong premise in #471.
That rate says the next one is found by looking rather than by waiting.
Method
Per solver family: set a distinctive value for each managed option,
solve()twice,pc.setUp(), then read the live PETSc objects — plus a defaults arm as control, because "reachable" is meaningless if the override happened to match the default. Findings are ranked by how invisible the failure is, not by the size of the substitution.Findings
preconditioner="fmg"was worse than the default because it bypassed the override latch"auto"warns about nothingStokesandStokes_Constraineddisagree about which options are reachable, in opposite directionssnes_rtolis owned by design but looks settableScalar and vector solver families came back clean — every option reachable. The whole defect class is localised to the saddle-point path, which makes the follow-up work much smaller than "audit everything".
Two things worth carrying forward
The report records both, because both cost a cycle:
Solver_N_prefixed view whilecustom_mg._configure_pcmgreads the global database with the live PC's full prefix. An unqualified record made every key look user-owned there, and the bundle silently stopped applying.Known gap in the method
The audit checked option keys for reachability. It did not check the solver's own properties for being accepted-and-inert —
solver.strategyvalidated three values and configured one, and it took @lmoresi pointing at it.penaltyandsaddle_preconditionerare the obvious remaining candidates and want a short targeted pass.Underworld development team with AI support from Claude Code