AI Review — barry/2026-07-17/rm-short-monitors

Deprecation of the short convergence monitors, plus test and reference-output cleanup.

Scope

Commits under review:

Findings

HIGH 1. 7e1291eed7e deletes 26 live _alt.out files whose content is not reproduced by the regenerated primaries

The commit message asserts these are "unneeded", but I could not reproduce that. Applying the old petscdiff normalization, 17 of the 28 deleted alts still differ structurally from their origin/main primaries — differences in iteration counts, function/gradient-evaluation counts, and active-set sizes, not floating-point digits, which normalization already masks. Comparing each deleted alt against the branch's new primary, 26 of 28 remain unmatched. Only src/snes/tutorials/output/ex12_p4est_full_q2_nonconformal_parallel_bddc_alt.out and ..._bddc_2_alt.out are genuinely subsumed.

Concrete examples:

• src/tao/bound/tutorials/output/plate2_2_alt.out — 21 vs 22 iterations, 25 vs 27 f/g evals, max: 4000 vs max: unlimited.
• src/snes/tutorials/output/ex58_2_alt.out — an extra 3 KSP Residual norm line; active sets 1671/4490 vs 854/4482, 775/3661 vs 424/3677, 368/2829 vs 277/2847.
• src/tao/unconstrained/tutorials/output/rosenbrock1_8_alt.out — 14 extra Tao iteration lines.

None of the owning tests were removed by this branch. Only ex7 suffix: 2, ex60 suffix: 9, and ex14 suffix: 2 were removed, and their .out files are correctly deleted alongside them. History also shows active maintenance rather than staleness — plate2_2_alt.out was last regenerated 2025-06-23 and ex12_44_alt.out 2026-06-05, both by unrelated MRs.

Full list of deletions that need justification or restoration:

src/snes/tutorials/output/ex12_44_alt.out
src/snes/tutorials/output/ex56_attach_mat_nearnullspace-1_bddc_approx_ml_alt.out
src/snes/tutorials/output/ex58_2_alt.out
src/tao/bound/tutorials/output/plate2_2_alt.out
src/tao/bound/tutorials/output/plate2_6_alt.out
src/tao/bound/tutorials/output/plate2_18_alt.out
src/tao/bound/tutorials/output/plate2_19_alt.out
src/tao/bound/tutorials/output/plate2_20_alt.out
src/tao/bound/tutorials/output/plate2f_1_alt.out
src/tao/bound/tutorials/output/plate2f_2_alt.out
src/tao/leastsquares/tests/output/chwirut2_1_alt.out
src/tao/leastsquares/tests/output/chwirut2_2_alt.out
src/tao/unconstrained/tutorials/output/eptorsion1_5_alt.out
src/tao/unconstrained/tutorials/output/eptorsion1_6_alt.out
src/tao/unconstrained/tutorials/output/minsurf1_7_alt.out
src/tao/unconstrained/tutorials/output/minsurf1_8_alt.out
src/tao/unconstrained/tutorials/output/rosenbrock1_8_alt.out
src/tao/unconstrained/tutorials/output/rosenbrock1_9_alt.out
src/tao/unconstrained/tutorials/output/rosenbrock1_10_alt.out
src/tao/unconstrained/tutorials/output/rosenbrock1_17_alt.out
src/tao/unconstrained/tutorials/output/rosenbrock1_18_alt.out
src/tao/unconstrained/tutorials/output/rosenbrock1_add_terms_l2_no_pre_alt_2.out
src/tao/unconstrained/tutorials/output/rosenbrock1_add_terms_l2_no_pre_alt_3.out
src/tao/unconstrained/tutorials/output/rosenbrock1_param_none_alt_2.out
src/tao/unconstrained/tutorials/output/rosenbrock1_taoterm_test_fd_hess_alt.out
src/ts/tutorials/output/ex12_2_alt.out

Most of these tests need datafilespath, superlu_dist, or non-default precision, so a standard CI run will not have exercised them — a green pipeline is not evidence they are unneeded.

Fix: restore the 26 files, or state in the commit message which configuration each was covering and why that configuration now matches the primary.

MEDIUM 2. lib/petsc/bin/petscdiff:133,135 — removing the < N normalization silently converts tolerance-tolerant comparisons into exact ones across the whole suite

The dropped first sed stage was:

s/< [-+ ]*[ 0-9][0-9]*\.*[0-9]*[eE][-+][0-9][0-9]*/XXX/g

Previously an expected < 1.e-11 normalized to XXX and matched any numeric actual; now it normalizes to < XXX and only matches another < …. This branch regenerates the KSP/SNES/Tao monitor references, but 85 other reference files rely on the removed masking for text this branch does not touch: 48 with L_2 Error: < 1.0e-11, 22 with Relative difference < 1e-12, 9 with min(a-b) < 1.0e-10, 6 with relative difference < 1e-. Only two new alt files were added (src/snes/tutorials/output/ex12_glvis_2d_hex_p2_alt.out, ex12_glvis_2d_tet_p2_alt.out), and they cover exactly one of those patterns.

13 source sites still emit a conditional < tol string and so become precision- and platform-fragile rather than tolerance-tolerant:

src/dm/tutorials/ex15.c:169
src/snes/interface/snes.c:4756
src/snes/utils/convest.c:271
src/tao/tests/taotermtest1.c:482
src/tao/tests/taotermtest1.c:483
src/tao/tests/taotermtest1.c:485
src/tao/tests/taotermtest1.c:486
src/tao/tests/taotermtest2.c:184
src/tao/tutorials/ex1.c:247
src/tao/tutorials/ex1.c:251
src/tao/tutorials/ex2.c:242
src/tao/tutorials/ex2.c:246
src/vec/vec/tests/ex46.c:103

Fix: either keep the < N stage — the monitor deprecation does not require removing it — or convert the remaining 13 sites to unconditional numeric prints the same way the KSP/SNES ones were converted, and regenerate the 85 affected references.

MEDIUM 3. 61 reference files still carry norm < 1.e-11 while their tests now pass -ksp_monitor/-snes_monitor

Latent, not live — all 61 sit behind TODO: markers (TODO: Matrix row/column sizes are not compatible with block size, TODO: broken) so they are skipped today. But combined with finding 2 they will fail the moment the TODO is lifted, and nothing records that.

Files: src/ksp/ksp/tutorials/output/ex72_7_d.out, 21 × ex72_7_matload_block_size-*, 36 × ex72_9_matload_block_size-* (including -5_c_alt), src/snes/tutorials/output/ex12_fas_newton_coarse_0.out, src/snes/tutorials/output/ex12_mg_newton_coarse_0.out.

Fix: regenerate them in this MR — they are mechanical < 1.e-11 → numeric substitutions — or note the debt in doc/changes/dev.md.

MEDIUM 4. src/ksp/ksp/tutorials/ex56.c:588-590 — rap_bs_hip silently loses monitor coverage and gains an incoherent filter

Every sibling in the testset (rap_bs, rap_bs_kokkos, rap_bs_cuda) had -rap_ksp_monitor re-added to its own args: when it was moved out of the shared testset args; rap_bs_hip alone did not. Its two references now contain 0 residual lines while cpu/kokkos have 11–13 and cuda 14–17.

The compensating sed at :589 is also wrong for one of the two sizes. It rewrites iterations 11|12 → iterations 12, but src/ksp/ksp/tutorials/output/ex56_nsize-1_rap_bs_hip.out expects iterations 15. For nsize-1 the substitution can only ever manufacture a mismatch — rewriting an actual 11 to 12 against an expected 15 — never mask one. Only nsize-8 (expected 12) is consistent with it.

Fix: give rap_bs_hip -rap_ksp_monitor like its siblings, and either drop the sed or scope it to nsize-8 with a comment explaining which HIP configuration produces the 11/12 split.

MEDIUM 5. src/tao/interface/taosolver.c / include/petsctao_deprecations.h:97 — TaoMonitorDefaultShort loses its Fortran binding with no replacement

The deleted TaoMonitorDefaultShort() had a full /*@ … @*/ block, so bfort generated taomonitordefaultshort_. Replacing it with a C static inline shim removes that stub — Fortran callers get a link error, not a deprecation warning.

This is not symmetric with the KSP/SNES cases: KSPMonitorResidualShort() and SNESMonitorDefaultShort() had plain /* … */ comments on origin/main ("Deprecated: Intentionally has no manual page"), so they never had stubs and lose nothing.

Fix: note the Fortran break in doc/changes/dev.md, or retain a thin /*@ @*/-documented C wrapper for the Tao case only.

MEDIUM 6. src/ksp/pc/tests/ex3.c:109 — unrelated behavioral fix bundled into a monitor-deprecation MR

args: -mat_type seqbaij → -mat_type seqsbaij under suffix: sor_seqsbaij. The fix looks correct — MatSOR_SeqSBAIJ exists at src/mat/impls/sbaij/seq/sbaij.c:1335, the matrix is the symmetric tridiagonal [-1 2 -1], and ignore_ltriangular defaults to PETSC_TRUE at sbaij.c:1935 — but it changes what the test exercises, shares output_file: output/ex3_1.out with three siblings, and has no accompanying reference change to show it was run. It is unrelated to short-monitor deprecation.

Fix: split into its own commit and confirm sor_seqsbaij actually passes against output/ex3_1.out.

MEDIUM 7. src/ksp/ksp/tutorials/network/ex1.c:316,321 — coverage traded away instead of fixing the underlying constraint

Both tests replace -ksp_monitor_short with -ksp_converged_reason, dropping residual-history checking entirely. The other ~60 converted sites use -ksp_monitor. The reason here is diff_args: -j (exact diff, no numeric normalization), which makes a full-precision monitor unportable — but the right fix is to drop -j for these tests, not to weaken the assertion.

Fix: remove diff_args: -j and use -ksp_monitor like every other converted test, or add a comment explaining why exact diffing is required for this example.

STYLE 8. include/petsctao_deprecations.h:97 — shim placed inconsistently with the KSP/SNES pattern

KSPMonitorResidualShort and SNESMonitorDefaultShort shims went into the main public headers (include/petscksp.h:596, include/petscsnes.h:132), immediately after their targets. The Tao one went into petsctao_deprecations.h instead. That header is unconditionally included at include/petsctao.h:562, so it works, but the three functions were deprecated in the same commit for the same reason and should be findable in the same place.

Fix: pick one location convention for the three, or note in the commit message why Tao differs.

LOW

LOW L1. doc/changes/dev.md:15 — entry understates the petscdiff change

Change petscdiff to treat < in expected output as text that must match instead of suppressing differences from numerical output

The removed sed stage ran on both pipelines — lib/petsc/bin/petscdiff:133 for file1 and :135 for file2 — so actual output containing < 1.e-11 was normalized too. The entry mentions only expected output.

LOW L2. Five Fortran guards left in block form around a single statement

Each was a 5-line if/else before the branch collapsed it; the surviving rank guard now wraps one write:

src/ksp/ksp/tutorials/ex11f.F90:193-195
src/ksp/ksp/tutorials/ex15f.F90:299-301
src/ksp/ksp/tutorials/ex2f.F90:353-355
src/ksp/ksp/tutorials/ex52f.F90:165-167
src/ksp/ksp/tests/ex62f.F90:274-276

AGENTS.md's brace rule is C-only and PETSc Fortran examples use both forms, hence LOW. ex1f.F90 and src/ksp/pc/tests/ex9f.F90 have no rank guard at all — the write is now unguarded, matching their prior behavior.

LOW L3. Deprecated option value is silently dropped when the new key is also present

PetscOptionsDeprecated_Private() (src/sys/objects/options.c:3373) does not overwrite an already-set new key, so -ksp_monitor ascii -ksp_monitor_short quietly discards the second. Same for -snes_monitor_short and -tao_monitor_short. Within Tao there is also an ordering interaction: src/tao/interface/taosolver.c:536 maps -tao_smonitor → -tao_monitor first, so a co-supplied -tao_monitor_short at :537 is dropped.

LOW L4. preconditioned_residual_short registry name removed with no deprecation path

src/ksp/ksp/interface/itregis.c:148 on origin/main. KSPMonitorRegister() is public API, so a lookup by that name now fails with no redirect. Documented in doc/changes/dev.md:99, which is why it stays LOW.

LOW L5. doc/manual/snes.md:879-881 — paragraph deleted with no replacement pointer

The surrounding text still describes -snes_monitor and -snes_monitor_lg_residualnorm; a reader coming from an older manual gets no note that -snes_monitor_short was folded into -snes_monitor.

LOW L6. Coverage caveat, not a defect

The tests behind findings 1 and 3 need datafilespath, HIP, superlu_dist, or non-double precision. A green standard pipeline does not exercise them, so it is not evidence either way.