fix(solve-report): SNES converged-reason table is off by one against PETSc

[lmoresi]

The bug

solve_report's SNES converged-reason table is shifted by one against PETSc, and claims a
code 1 that PETSc does not define:

code	PETSc	reported as
2	CONVERGED_FNORM_ABS	CONVERGED_FNORM_RELATIVE
3	CONVERGED_FNORM_RELATIVE	CONVERGED_SNORM_RELATIVE
4	CONVERGED_SNORM_RELATIVE	CONVERGED_ITS
5	CONVERGED_ITS	UNKNOWN_5

Why the direction matters

A solve that stopped on the step norm — the weakest criterion, and what a stalled
viscoplastic solve reports — was labelled CONVERGED_ITS. A genuine residual convergence
was labelled CONVERGED_SNORM_RELATIVE.

Reading a difficulty report is how a continuation driver decides whether a parameter
station is reachable, so a mislabelled convergence is exactly the kind of error that puts
false rescues on a regime map without anyone noticing.

Found while checking why a hard Drucker–Prager station was reporting CONVERGED_ITS on a
solve that had no iteration cap in play. It had actually converged on the step norm — a
different and much weaker claim.

Also fixed

DIVERGED_OBJECTIVE_DOMAIN (-13) and DIVERGED_OBJECTIVE_NANORINF (-14) were unmapped
and surfaced as UNKNOWN_n; -4 is DIVERGED_FUNCTION_NANORINF in PETSc, not
DIVERGED_FNORM_NAN.

How it survived

The table is hand-written on purpose — the module must import without the Cython
extension present — so it can drift. The existing test pinned it to itself:

assert reason_string(2) == "CONVERGED_FNORM_RELATIVE"    # asserts the bug

The KSP table sitting next to it was checked against petsc4py and is correct. Only the
SNES one was never pinned.

test_snes_reason_table_matches_petsc now checks both directions: every label matches the
enum, and every reason PETSc can return is mapped — so a PETSc renumbering or a future
hand-edit fails the test instead of silently mislabelling solves.

Verified against petsc4py 3.25; the reason-table tests pass.

Underworld development team with AI support from Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[lmoresi]

Adversarial review

Reviewed against the standing rule that a regression test must be shown to fail without its fix, and that a table pinned to itself proves nothing.

Verified: the bug is real and the fix is right

I checked the pre-fix table against petsc4py's enum directly in the build environment. Six entries were wrong, not one:

code	old label	actual PETSc name
1	CONVERGED_FNORM_ABS	not a SNES reason at all
2	CONVERGED_FNORM_RELATIVE	CONVERGED_FNORM_ABS
3	CONVERGED_SNORM_RELATIVE	CONVERGED_FNORM_RELATIVE
4	CONVERGED_ITS	CONVERGED_SNORM_RELATIVE
0	ITERATING	CONVERGED_ITERATING
-4	DIVERGED_FNORM_NAN	DIVERGED_FUNCTION_NANORINF

So test_snes_reason_table_matches_petsc genuinely fails on the old table — it is a real regression test, not a restatement. The corrected table matches the enum exactly in both directions (every table entry is a PETSc reason; every PETSc reason is mapped), which is the property that actually keeps a hand-copy honest.

The consequence stated in the commit message checks out: code 3 is a genuine residual convergence and was being reported as CONVERGED_SNORM_RELATIVE, while code 4 — the step-norm criterion, which is what a stalled viscoplastic solve reports — was being reported as CONVERGED_ITS.

Finding (fixed in this PR)

vars(PETSc.SNES.ConvergedReason) yields two names for code 0, CONVERGED_ITERATING and ITERATING. The new test collapsed that with enum_names.setdefault(value, name), so it asserted against whichever alias vars() enumerated first. That ordering is a petsc4py implementation detail, not a guarantee — a build that enumerated them the other way would have failed a level_1/tier_a test on a purely cosmetic difference, in CI, for no real defect. Now fixed by collecting the alias set per code and asserting membership.

Worth noting the existing test_ksp_reason_table_matches_petsc has the same hazard and sidesteps it by continue-ing on code 0. KSP.ConvergedReason carries the identical alias pair. Not changed here — out of scope for this PR — but it is the same one-line improvement if anyone is in that file next.

Note for anyone reading older logs

Any solve_report label printed before this fix is shifted. In a pre-2026-07-27 log, CONVERGED_FNORM_RELATIVE actually meant CONVERGED_FNORM_ABS, and CONVERGED_ITS meant CONVERGED_SNORM_RELATIVE — i.e. a step-norm stall reported as an iteration-count success. That matters for any continuation driver that branched on the reason string, and for re-reading campaign logs.

Not covered

The table is still a hand-copy that only the test keeps aligned; nothing prevents the same drift in the compact map in SolverBaseClass._convergence_reasons that this one was duplicated from. Worth a follow-up to check whether that copy is also shifted.

Underworld development team with AI support from Claude Code

[lmoresi]

Follow-up found and fixed in this PR: the table was duplicated, and only one copy was corrected

My own review note above ("nothing prevents the same drift in the compact map in SolverBaseClass._convergence_reasons that this one was duplicated from") turned out to be the actual live bug, not a hypothetical. That second copy had the identical off-by-one and was untouched by the original fix:

1: ("CONVERGED_FNORM_ABS", "||F|| < atol"),          # no such SNES code
2: ("CONVERGED_FNORM_RELATIVE", ...),                 # actually FNORM_ABS
4: ("CONVERGED_ITS", "Maximum iterations reached"),   # actually the step-norm stop
-4: ("DIVERGED_FNORM_NAN", ...),                      # actually FUNCTION_NANORINF

It also had no entries for -13/-14, so an objective-domain or objective-NaN divergence printed UNKNOWN(-13).

This matters because it is not a dead table. It feeds get_convergence_diagnostics (the user-facing "NAME - explanation" string) and supplies the name _warn_on_divergence prints on every diverged solve. Fixing only solve_report would have left the two copies contradicting each other about what code 2 means — a worse state than one consistently-wrong table, and exactly the drift pattern the Charter's DRY clause is about.

test_1055 now pins both copies to petsc4py's enum and to each other, so neither can drift alone.

Verified it fails without the fix, against a real build rather than by inspection: running the updated test against an environment carrying the old solver table gives

AssertionError: 1 (CONVERGED_FNORM_ABS) is not a PETSc SNES reason at all

The good news for _warn_on_divergence specifically: the negative codes it actually prints were mostly right already (-3 DIVERGED_LINEAR_SOLVE, -5, -6 all correct), so divergence warnings in existing logs are trustworthy. It is the converged labels, and anything reading get_convergence_diagnostics, that were wrong.

Left deliberately unfixed: the two copies are still two copies. Collapsing them (the solver deriving names from REASON_STRINGS and holding only the explanations) is the real DRY fix, but it edits solver code for a cosmetic gain and is out of scope here — the test now makes the duplication safe rather than merely present.

Underworld development team with AI support from Claude Code