From 2f9b928f586721121be645d911da5b9a98551017 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Mon, 27 Jul 2026 19:12:21 +1000 Subject: [PATCH 1/3] fix(solve-report): SNES converged-reason table was off by one against PETSc Every positive SNES reason was shifted by one slot, and the table claimed a code 1 that PETSc does not define: reason 2 PETSc CONVERGED_FNORM_ABS reported as CONVERGED_FNORM_RELATIVE reason 3 PETSc CONVERGED_FNORM_RELATIVE reported as CONVERGED_SNORM_RELATIVE reason 4 PETSc CONVERGED_SNORM_RELATIVE reported as CONVERGED_ITS reason 5 PETSc CONVERGED_ITS reported as UNKNOWN_5 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, while 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 the kind of error that quietly puts false rescues on a regime map. Found while checking why a hard plastic station was reporting CONVERGED_ITS. Also adds DIVERGED_OBJECTIVE_DOMAIN (-13) and DIVERGED_OBJECTIVE_NANORINF (-14), which were unmapped and surfaced as UNKNOWN_n, and corrects -4 to its PETSc name DIVERGED_FUNCTION_NANORINF. The table is hand-written on purpose -- the module must import without the Cython extension -- so it can drift, and the existing test pinned it to ITSELF (reason_string(2) == 'CONVERGED_FNORM_RELATIVE') rather than to the enum, which is how this survived. The KSP table next to it WAS checked against petsc4py and was correct. test_snes_reason_table_matches_petsc now checks both directions: every label matches the enum, and every reason PETSc can return is mapped. Underworld development team with AI support from Claude Code --- src/underworld3/systems/solve_report.py | 24 ++++++++++------ tests/test_1055_solve_report.py | 37 ++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 10 deletions(-) diff --git a/src/underworld3/systems/solve_report.py b/src/underworld3/systems/solve_report.py index 8cdd6358..bc5c5b3f 100644 --- a/src/underworld3/systems/solve_report.py +++ b/src/underworld3/systems/solve_report.py @@ -14,19 +14,23 @@ from dataclasses import dataclass from typing import Optional, Tuple -# PETSc SNESConvergedReason codes -> short names. Mirrors the compact map in -# SolverBaseClass._convergence_reasons; duplicated here so this module imports without the -# Cython solver extension present. +# PETSc SNESConvergedReason codes -> short names. Duplicated here (rather than read from +# petsc4py) so this module imports without the Cython solver extension present -- which +# means it can drift, and it HAD: every positive code was shifted by one, so a solve that +# stopped on the STEP norm (the weakest criterion, and what a stalled plastic solve +# reports) was labelled CONVERGED_ITS, and a genuine residual convergence was labelled +# CONVERGED_SNORM_RELATIVE. There is no code 1. test_1055 now checks this table against +# petsc4py's enum, which is the only thing that can keep a hand-copy honest. REASON_STRINGS = { - 1: "CONVERGED_FNORM_ABS", - 2: "CONVERGED_FNORM_RELATIVE", - 3: "CONVERGED_SNORM_RELATIVE", - 4: "CONVERGED_ITS", - 0: "ITERATING", + 0: "CONVERGED_ITERATING", + 2: "CONVERGED_FNORM_ABS", + 3: "CONVERGED_FNORM_RELATIVE", + 4: "CONVERGED_SNORM_RELATIVE", + 5: "CONVERGED_ITS", -1: "DIVERGED_FUNCTION_DOMAIN", -2: "DIVERGED_FUNCTION_COUNT", -3: "DIVERGED_LINEAR_SOLVE", - -4: "DIVERGED_FNORM_NAN", + -4: "DIVERGED_FUNCTION_NANORINF", -5: "DIVERGED_MAX_IT", -6: "DIVERGED_LINE_SEARCH", -7: "DIVERGED_INNER", @@ -34,6 +38,8 @@ -9: "DIVERGED_DTOL", -10: "DIVERGED_JACOBIAN_DOMAIN", -11: "DIVERGED_TR_DELTA", + -13: "DIVERGED_OBJECTIVE_DOMAIN", + -14: "DIVERGED_OBJECTIVE_NANORINF", } diff --git a/tests/test_1055_solve_report.py b/tests/test_1055_solve_report.py index be3de21d..bda37fca 100644 --- a/tests/test_1055_solve_report.py +++ b/tests/test_1055_solve_report.py @@ -145,7 +145,10 @@ def test_resume_is_lossless_and_same_termination(): @pytest.mark.level_1 @pytest.mark.tier_a def test_solve_report_helpers(): - assert reason_string(2) == "CONVERGED_FNORM_RELATIVE" + # 2 is CONVERGED_FNORM_ABS in PETSc. This line previously asserted + # CONVERGED_FNORM_RELATIVE — it pinned the table to itself rather than to the enum, + # which is how the off-by-one survived. See test_snes_reason_table_matches_petsc. + assert reason_string(2) == "CONVERGED_FNORM_ABS" assert reason_string(-5) == "DIVERGED_MAX_IT" assert reason_string(999).startswith("UNKNOWN") assert contraction([50.0, 1e-3, 1e-6, 1e-9]) is not None @@ -261,3 +264,35 @@ def test_ksp_reason_table_matches_petsc(): assert label == f"KSP_{enum_names[code]}", (code, label, enum_names[code]) assert ksp_reason_string(-3) == "KSP_DIVERGED_MAX_IT" assert ksp_reason_string(999).startswith("KSP_UNKNOWN") + + +@pytest.mark.level_1 +@pytest.mark.tier_a +def test_snes_reason_table_matches_petsc(): + """The SNES table is hand-written for the same reason as the KSP one, and unlike the + KSP one it was never pinned to the enum — so it drifted. Every positive code was + shifted by one: a solve that stopped on the STEP norm (the weakest criterion, and + what a stalled viscoplastic solve reports) was labelled CONVERGED_ITS, while a + genuine residual convergence was labelled CONVERGED_SNORM_RELATIVE. Reading a + difficulty report is how continuation drivers decide whether a station is reachable, + so the labels have to be the real ones.""" + from petsc4py import PETSc + from underworld3.systems.solve_report import REASON_STRINGS, reason_string + + enum_names = {} + for name, value in vars(PETSc.SNES.ConvergedReason).items(): + if isinstance(value, int) and not name.startswith("_"): + enum_names.setdefault(value, name) + + for code, label in REASON_STRINGS.items(): + assert code in enum_names, f"{code} ({label}) is not a PETSc SNES reason at all" + assert label == enum_names[code], (code, label, enum_names[code]) + + # Every reason PETSc can return must be nameable — an UNKNOWN_n in a report is a + # gap in the table, and the ones that went missing were real diverged states. + for code in enum_names: + assert code in REASON_STRINGS, f"PETSc reason {code} ({enum_names[code]}) unmapped" + + assert reason_string(4) == "CONVERGED_SNORM_RELATIVE" + assert reason_string(5) == "CONVERGED_ITS" + assert reason_string(999).startswith("UNKNOWN") From 96b6722c664b668bf0eeadd8ea689504e736fde3 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Tue, 28 Jul 2026 11:10:58 +1000 Subject: [PATCH 2/3] test(solve-report): match the SNES reason table against every petsc4py alias A PETSc reason code can carry more than one petsc4py spelling: 0 is both CONVERGED_ITERATING and ITERATING. The new table test kept one name per code via setdefault, so it asserted against whichever name vars() happened to yield first. That is not a petsc4py guarantee, and a build that enumerated the aliases the other way would have failed a level_1/tier_a test on a purely cosmetic difference. Collect the alias set per code and assert membership instead. The existing KSP test sidesteps the same hazard by skipping code 0; this covers it rather than skipping it. Underworld development team with AI support from Claude Code --- tests/test_1055_solve_report.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/test_1055_solve_report.py b/tests/test_1055_solve_report.py index bda37fca..509f312f 100644 --- a/tests/test_1055_solve_report.py +++ b/tests/test_1055_solve_report.py @@ -279,19 +279,23 @@ def test_snes_reason_table_matches_petsc(): from petsc4py import PETSc from underworld3.systems.solve_report import REASON_STRINGS, reason_string + # A code can carry more than one petsc4py spelling (0 is both CONVERGED_ITERATING + # and ITERATING), so collect every alias: picking one via setdefault would pin the + # test to the order vars() happens to yield, which is not a petsc4py guarantee. enum_names = {} for name, value in vars(PETSc.SNES.ConvergedReason).items(): if isinstance(value, int) and not name.startswith("_"): - enum_names.setdefault(value, name) + enum_names.setdefault(value, set()).add(name) for code, label in REASON_STRINGS.items(): assert code in enum_names, f"{code} ({label}) is not a PETSc SNES reason at all" - assert label == enum_names[code], (code, label, enum_names[code]) + assert label in enum_names[code], (code, label, sorted(enum_names[code])) # Every reason PETSc can return must be nameable — an UNKNOWN_n in a report is a # gap in the table, and the ones that went missing were real diverged states. for code in enum_names: - assert code in REASON_STRINGS, f"PETSc reason {code} ({enum_names[code]}) unmapped" + assert code in REASON_STRINGS, ( + f"PETSc reason {code} ({sorted(enum_names[code])}) unmapped") assert reason_string(4) == "CONVERGED_SNORM_RELATIVE" assert reason_string(5) == "CONVERGED_ITS" From a58667b821c912c46c9c32d5165691a261dd91ce Mon Sep 17 00:00:00 2001 From: lmoresi Date: Tue, 28 Jul 2026 11:13:57 +1000 Subject: [PATCH 3/3] fix(solvers): the solver's own reason table had the same off-by-one The table was duplicated: solve_report.REASON_STRINGS (fixed in the previous commit) and SolverBaseClass._convergence_reasons, which adds a one-line gloss for get_convergence_diagnostics and supplies the name _warn_on_divergence prints. Only the first copy was corrected, which left the two disagreeing about what code 2 means -- worse than one consistently wrong table. Same shift here: there is no code 1, code 4 is the step-norm stop rather than CONVERGED_ITS, and -4 is DIVERGED_FUNCTION_NANORINF. The two objective-function divergence codes were missing, so those solves printed UNKNOWN(-13)/UNKNOWN(-14). test_1055 now pins BOTH copies to petsc4py's enum and to each other, so neither can drift alone. Verified the added assertions fail against a build carrying the old solver table. Underworld development team with AI support from Claude Code --- .../cython/petsc_generic_snes_solvers.pyx | 23 +++++++++++-------- tests/test_1055_solve_report.py | 15 ++++++++++++ 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/underworld3/cython/petsc_generic_snes_solvers.pyx b/src/underworld3/cython/petsc_generic_snes_solvers.pyx index 0fda2ebd..e71f5f4e 100644 --- a/src/underworld3/cython/petsc_generic_snes_solvers.pyx +++ b/src/underworld3/cython/petsc_generic_snes_solvers.pyx @@ -1324,22 +1324,25 @@ class SolverBaseClass(uw_object): return None - # SNES convergence reasons (PETSc documentation): code -> (NAME, explanation). - # Single source for both get_convergence_diagnostics (formats - # "NAME - explanation") and _warn_on_divergence (uses NAME only). + # SNES convergence reasons: code -> (NAME, explanation). The NAMES are the same + # table as solve_report.REASON_STRINGS, kept here with an explanation string for + # get_convergence_diagnostics (formats "NAME - explanation") and _warn_on_divergence + # (uses NAME only). Both copies are pinned to petsc4py's enum by test_1055 — the + # positive codes here were shifted by one until 2026-07 (there is no code 1, and a + # step-norm stop was reported as CONVERGED_ITS). _convergence_reasons = { # Positive reasons = converged - 1: ("CONVERGED_FNORM_ABS", "||F|| < atol"), - 2: ("CONVERGED_FNORM_RELATIVE", "||F|| < rtol*||F_initial||"), - 3: ("CONVERGED_SNORM_RELATIVE", "||x|| < stol"), - 4: ("CONVERGED_ITS", "Maximum iterations reached"), + 2: ("CONVERGED_FNORM_ABS", "||F|| < atol"), + 3: ("CONVERGED_FNORM_RELATIVE", "||F|| < rtol*||F_initial||"), + 4: ("CONVERGED_SNORM_RELATIVE", "||x|| < stol"), + 5: ("CONVERGED_ITS", "Maximum iterations reached"), # Zero = still iterating (shouldn't see after solve) - 0: ("ITERATING", "Still iterating (unexpected after solve)"), + 0: ("CONVERGED_ITERATING", "Still iterating (unexpected after solve)"), # Negative reasons = diverged -1: ("DIVERGED_FUNCTION_DOMAIN", "Function domain error"), -2: ("DIVERGED_FUNCTION_COUNT", "Too many function evaluations"), -3: ("DIVERGED_LINEAR_SOLVE", "Linear solver failed"), - -4: ("DIVERGED_FNORM_NAN", "||F|| is Not-a-Number"), + -4: ("DIVERGED_FUNCTION_NANORINF", "||F|| is Not-a-Number or infinite"), -5: ("DIVERGED_MAX_IT", "Maximum iterations exceeded"), -6: ("DIVERGED_LINE_SEARCH", "Line search failed"), -7: ("DIVERGED_INNER", "Inner solve failed"), @@ -1347,6 +1350,8 @@ class SolverBaseClass(uw_object): -9: ("DIVERGED_DTOL", "||F|| increased by divtol"), -10: ("DIVERGED_JACOBIAN_DOMAIN", "Jacobian calculation failed"), -11: ("DIVERGED_TR_DELTA", "Trust region delta too small"), + -13: ("DIVERGED_OBJECTIVE_DOMAIN", "Objective function domain error"), + -14: ("DIVERGED_OBJECTIVE_NANORINF", "Objective is Not-a-Number or infinite"), } def _warn_on_divergence(self, phase="solve"): diff --git a/tests/test_1055_solve_report.py b/tests/test_1055_solve_report.py index 509f312f..90d68327 100644 --- a/tests/test_1055_solve_report.py +++ b/tests/test_1055_solve_report.py @@ -300,3 +300,18 @@ def test_snes_reason_table_matches_petsc(): assert reason_string(4) == "CONVERGED_SNORM_RELATIVE" assert reason_string(5) == "CONVERGED_ITS" assert reason_string(999).startswith("UNKNOWN") + + # The solver carries a SECOND copy of this table (code -> (NAME, explanation)) so + # its diagnostics can add a one-line gloss. It had the identical off-by-one, and + # fixing only one copy would leave the two disagreeing — so pin both to the enum + # and to each other. + from underworld3.systems import Stokes + + solver_table = Stokes._convergence_reasons + for code, (label, _explanation) in solver_table.items(): + assert code in enum_names, f"{code} ({label}) is not a PETSc SNES reason at all" + assert label in enum_names[code], (code, label, sorted(enum_names[code])) + for code in enum_names: + assert code in solver_table, ( + f"PETSc reason {code} ({sorted(enum_names[code])}) unmapped in the solver") + assert {c: n for c, (n, _) in solver_table.items()} == REASON_STRINGS