Rotated solve: stop counting the pinned row, and report the verdict - #548
Rotated solve: stop counting the pinned row, and report the verdict#548lmoresi wants to merge 1 commit into
Conversation
Two defects, both of which made a converged rotated solve report that it had not converged. The direct path pins one pressure DOF to fix the gauge — the row is replaced by the identity and the rhs zeroed — but the residual kept counting that row. The loop therefore measured itself against a floor it had itself defined as unreachable, and ran to max_it. Measured on a fault contact problem: the velocity residual reached 6e-12 at the FIRST increment and the entire remaining |F| was the pinned DOF alone, bit-identical for eight further iterations that changed nothing (the velocity residual was exactly 0.0 from the second on). The pin is now decided once, before the residual closure is built, and excluded from the residual for the same reason normal_rows are: it is not an equation the solve is trying to satisfy, so it is not evidence about convergence. Separately, the loop never published a reason on the SNES. Every SNES call inside it evaluates one residual or tangent, so the SNES's own reason describes a linear step and stays 0 for the solve as a whole — and petsc_generic_snes_solvers decides convergence from `snes.getConvergedReason() > 0`. Every rotated solve was therefore reported as unconverged, including ones that converged at rel 1e-8. The loop now sets FNORM_RELATIVE / SNORM_RELATIVE on a clean exit and DIVERGED_MAX_IT / DIVERGED_LINE_SEARCH otherwise. The regression test covers both paths, because the pin exists only in the direct one; it fails on both before this change. Underworld development team with AI support from Claude Code
There was a problem hiding this comment.
Pull request overview
Fixes rotated free-slip solve convergence reporting so downstream code can reliably interpret snes.getConvergedReason() for the rotated nonlinear loop (and so the LU path doesn’t prevent convergence by counting a pinned row).
Changes:
- Hoists the LU pressure-pin decision and excludes the pinned row from the rotated residual norm.
- Publishes the rotated loop’s convergence/divergence verdict back onto the solver’s SNES object.
- Adds a regression test covering both iterative and LU rotated paths for correct convergence reporting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/underworld3/utilities/rotated_bc.py |
Aligns residual evaluation and convergence reporting with LU pinning and rotated-loop semantics. |
tests/test_1018_rotated_freeslip.py |
Adds a regression test ensuring rotated solves report convergence correctly for both paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| _SNES_CONVERGED_FNORM_RELATIVE = 2 | ||
| _SNES_CONVERGED_SNORM_RELATIVE = 4 | ||
| _SNES_DIVERGED_MAX_IT = -5 | ||
| _SNES_DIVERGED_LINE_SEARCH = -6 |
| reason = int(s.snes.getConvergedReason()) | ||
| assert reason > 0, ( | ||
| f"the rotated solve reports reason {reason}; a converged solve must " | ||
| "say so, because the generic path decides convergence from this") | ||
|
|
|
Note on the existing reporting channel, so this reads as complementary rather than duplicative.
What this PR fixes is that the two disagreed, and the library itself trusted the wrong one: converged = self.snes is not None and self.snes.getConvergedReason() > 0so an internal check was reading a value the rotated path never set. After this change the SNES reason agrees with the info dict instead of contradicting it, and the internal check becomes correct without callers having to know which of the two channels applies to their solve. The skill's table entry ("rotated free-slip Stokes 'not converged' — |
Two defects, both of which made a converged rotated solve report that it had not converged. Found while chasing what looked like a nonlinear stall in a fault-contact model.
1. The direct path counts a row it removed from the system
_naive_pressure_pinfixes the pressure gauge for the LU path by replacing one pressure row with the identity and zeroing its rhs.rotated_residualdid not exclude that row, the way it already excludesnormal_rows. The loop therefore measured itself against a floor it had itself defined as unreachable, and ran tomax_it.The residual breakdown on a fault-contact problem, per outer iteration:
The velocity problem is solved at the first increment. Everything after it is the pinned DOF alone — bit-identical, with the increment exactly zero, for eight further iterations that changed nothing. Confirmed by index: the residual's single non-zero row is exactly
_naive_pressure_pin(dm).The tell that it was bookkeeping rather than physics: the reported
rel |F̂| = 1.89e-05is identical to three figures with the interface law at η_f = 0, 0.005, 0.05 and 0.5 — a residual that does not move when the physics is deleted is not a physical stall.The pin is now decided once, before the residual closure is built, and excluded from the residual for the same reason
normal_rowsare: it is not an equation the solve is trying to satisfy, so it is not evidence about convergence. The LU branch reuses that same decision rather than recomputing it.2. The loop never published its verdict
Every SNES call inside the rotated loop evaluates one residual or tangent, so
snes.getConvergedReason()describes a linear step and stays 0 for the solve as a whole.petsc_generic_snes_solversdecides convergence fromsnes.getConvergedReason() > 0, so every rotated solve was reported as unconverged — including ones that exited cleanly at rel 1e-8.The loop now sets
CONVERGED_FNORM_RELATIVE/CONVERGED_SNORM_RELATIVEon a clean exit, andDIVERGED_MAX_IT/DIVERGED_LINE_SEARCHotherwise.Why this matters beyond the noise
A reason code that reads 0 on a converged solve is not merely cosmetic here: it is the value the library itself uses to decide whether a solve worked, and it trains users to ignore the check. In the campaign that found this, four fault models were reported as unconverged and their results treated as provisional; the fields were converged the whole time, which is why an exact LU velocity block and an unconverged GMRES one agreed on slip to four significant figures.
Tests
test_the_rotated_solve_reports_its_own_convergence, parametrised over both paths because the pin exists only in the direct one. Both parametrisations fail before this change and pass after.Passing:
test_1018,test_1061,test_1062,test_1065(41);test_1058,test_0846,test_0847,test_0848,test_0203,test_1021(63) — the suites that read the converged reason downstream;test_1064andtest_1066at np=2 (11).Underworld development team with AI support from Claude Code