Skip to content

fix(solve-report): honest rotated-path reports, KSP reason namespace, resume-anchor lifecycle#437

Merged
lmoresi merged 1 commit into
developmentfrom
bugfix/solve-report-fixes
Jul 27, 2026
Merged

fix(solve-report): honest rotated-path reports, KSP reason namespace, resume-anchor lifecycle#437
lmoresi merged 1 commit into
developmentfrom
bugfix/solve-report-fixes

Conversation

@lmoresi

@lmoresi lmoresi commented Jul 27, 2026

Copy link
Copy Markdown
Member

Summary

Quick-deploy fixes for the findings of the post-merge adversarial review of PR #377 (posted on that PR). All three MAJOR and all three MINOR findings are addressed; the NOTE-level items are documented in the review thread and the two probe-contract ones are guarded here.

What was broken (all demonstrated live in the review)

  1. Nonlinear rotated free-slip solves left solve_report = None_capture_rotated_report read the nonlinear dict's ksp_its (a per-Newton list) as an int; the TypeError was swallowed by a blanket except Exception, so a successful 7-iteration solve recorded nothing.
  2. estimate_difficulty on a rotated solver silently ignored its cap — the rotated dispatch returns before _snes_solve_with_retries, so a max_nl_its=1 probe ran the full unbounded Newton loop and returned None.
  3. The resume anchor had no lifecycle — a warm probe on an already-converged state armed an unreachable target at tolerance*‖F_converged‖, and no event (convergence, physics change, solver rebuild) ever cleared it.
  4. KSP reason codes were rendered with SNES names — the integers overlap but name different outcomes (-3 is KSP DIVERGED_MAX_IT but SNES DIVERGED_LINEAR_SOLVE); a diagnostic that mislabels failure modes is worse than none.
  5. A solve that raised left the previous converged report in place for recovery logic to misread.
  6. The linear rotated report was hollow (fnorm always NaN — no residual in the result dict).

The fixes

  • _capture_rotated_report handles both rotated result dicts (list vs int ksp_its, nonlinear_iterations, outer converged verdict) and renders reasons with a new KSP-namespace table in solve_report.py (names carry a KSP_ prefix so report strings are unambiguous). The blanket swallow is removed — the dicts and the reader are a contract, and the swallow is what masked the breakage.
  • Both rotated paths now return real residuals: the linear solve computes the true rotated residual ‖·û − b̂‖ explicitly (one matvec — preonly/LU never computes a norm and iterative norms can be preconditioned); the nonlinear loop returns its final/initial ‖F̂‖.
  • estimate_difficulty raises NotImplementedError with rotated free-slip BCs, and TypeError on zero_init_guess / divergence_retries kwargs (a capped probe ends DIVERGED_MAX_IT by design — retrying on it would run multiples of the advertised cap).
  • Anchor lifecycle: a converged probe clears the anchor (chain complete — also neutralises the warm-first-call poisoning); _reset clears it (a torn-down solver is a different problem).
  • Report capture moved into the finally of _snes_solve_with_retries: a raising solve leaves an honest report of the failed attempt.
  • Charter §4: the three optional-field swallows in _capture_solve_report now state their sanctioned failure mode.

Validation

  • test_1055_solve_report.py: 10/10 — including 5 new cases: rotated linear report (KSP namespace, finite residual), rotated nonlinear report (nl/ksp counts match the result dict, outer verdict honoured), rotated + kwarg rejection, converged-warm anchor hygiene (no grinding, no armed anchor), and the KSP table pinned against petsc4py's ConvergedReason enum.
  • test_1018_rotated_freeslip.py + test_1019_boundary_flux.py: 21/21 (rotated solve tail touched).
  • Full level_1 and tier_a gate: 479 passed, 0 failed (17 skipped, 1 xfail).

Review provenance

Review thread with probe evidence: #377 (post-merge adversarial review comment). Note for PR #418: this PR adds "rnorm" / "rnorm0" keys to the rotated result dicts — the #418 rebase should preserve them.

Underworld development team with AI support from Claude Code

… anchor lifecycle (PR #377 review)

Fixes the three MAJOR and three MINOR findings from the post-merge adversarial
review of PR #377:

1. _capture_rotated_report now honours both rotated result dicts: the nonlinear
   loop's ksp_its LIST (summed), nonlinear_iterations, and the outer converged
   verdict. Previously a TypeError on the list was swallowed by a blanket
   except, leaving solve_report = None after every nonlinear rotated solve.
   The blanket swallow is gone: a malformed dict now raises (the dicts and the
   reader are a contract; the swallow is what masked the breakage).

2. KSP converged-reason codes get their own table (KSP_ prefix) — they were
   rendered with SNES names that share integers but mean different outcomes
   (e.g. -3: KSP DIVERGED_MAX_IT vs SNES DIVERGED_LINEAR_SOLVE). A test pins
   the table against petsc4py's enum.

3. Both rotated paths report a real residual: the linear solve computes the
   true rotated residual (preonly/LU never computes a norm; iterative norms
   can be preconditioned), the nonlinear loop returns its final/initial |F|.

4. estimate_difficulty raises NotImplementedError with rotated free-slip BCs
   (that path solves outside self.snes, so the cap and anchor cannot reach
   it — it previously ran unbounded and returned None), and rejects
   zero_init_guess / divergence_retries kwargs that would break the probe
   contract.

5. Anchor lifecycle: a CONVERGED probe clears the resume anchor (a warm first
   call on a converged state previously armed an unreachable target at
   tolerance*|F_converged| and ground extra iterations with a misleading
   exit), and _reset clears it (a torn-down solver is a different problem).

6. Report capture moved into the finally of _snes_solve_with_retries: a solve
   that raises now leaves an honest report of the failed attempt instead of
   the previous solve's converged one.

Tests: 5 new cases in test_1055 (rotated linear + nonlinear reports, rotated /
kwarg rejection, converged-warm anchor hygiene, KSP table pinned to petsc4py).

Underworld development team with AI support from Claude Code
Copilot AI review requested due to automatic review settings July 27, 2026 05:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lmoresi

lmoresi commented Jul 27, 2026

Copy link
Copy Markdown
Member Author

Verification: the original attack probes, rerun against this fix

The adversarial review of PR #377 left its probe scripts; rerunning the three that demonstrated the MAJOR findings, unmodified, against this branch's build:

p2 (rotated reports) — linear: SolveReport(KSP_CONVERGED_RTOL, nl=1, ksp=1, |F|=4.059e-07) (was |F|=nan, SNES-named reason); nonlinear: SolveReport(KSP_CONVERGED_RTOL, nl=7, ksp=7, |F|=8.426e-08) with history trail length 1 (was None and an empty trail); estimate_difficulty(max_nl_its=1) on the rotated solver now raises NotImplementedError at the exact line where it previously ran 7 unbounded iterations and returned None.

p3c (anchor lifecycle) — the script now crashes formatting the anchor because it is None after a converged chain: the poisoned/stale anchor it was written to expose no longer exists.

p5 (stale report on raised solve) — after an injected mid-solve failure: report after FAILED solve: SolveReport(ITERATING, nl=0, ksp=0, ...), stale? False | converged flag: False (was the previous solve's converged=True report).

p4 (np2 parallel) — plain / bounded / resume / rotated-annulus reports all rank-consistent, anchors consistent, resume chain still terminates at the anchored point (|F|=9.455e-08 matching the uninterrupted reference), rotated report in the KSP namespace.

Gate: level_1+tier_a 479 passed, 0 failed in the worktree; test_1018 + test_1019 21/21.

Underworld development team with AI support from Claude Code

@lmoresi
lmoresi merged commit e5274a1 into development Jul 27, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants