Branch review — `jose/ksp-feature-idr`

Add KSPIDR (IDR(s) Krylov solver) and PetscRandom prefix helpers.

Source: HEAD
Target (DEST): origin/main — merge-base is not an ancestor of origin/release
Scope: 12 code/doc files (+ 6 .out reference files, content not reviewed)
Changes: (1) new KSPIDR solver under src/ksp/ksp/impls/idr/; (2) PetscRandomAppendOptionsPrefix() / PetscRandomGetOptionsPrefix()

Summary

Severity	Count	Status
Critical	0	—
High	0	—
Medium	1	Open
Style	0	—
Low	3	Open (optional)

Verified

clang-format clean on all four touched .c files
{cite}, \:, and options-key syntax match repo conventions
Docstring section order and parameter alignment correct
All three new bib entries are referenced (no orphans)
ex2_help.out regeneration justified (adds idr to the KSP type list)
Output files for all new tests are committed
IDR(s) recurrence, M forward-substitution, biorthogonalization, and omega/angle-stabilization math check out
Right-preconditioning guess/unwind path matches KSPBCGS

Medium

Medium `KSP_NORM_NONE` not supported with `PC_LEFT`, unlike sibling solver `KSPBCGS` src/ksp/ksp/impls/idr/idr.c:580–583

IDR registered KSP_NORM_NONE only for PC_RIGHT, while KSPBCGS (bcgs.c:214–217) registers it for both PC_LEFT and PC_RIGHT. Since IDR(1) is mathematically BCGS and the solve body already supports the combination (every norm is guarded by if (ksp->normtype != KSP_NORM_NONE) and left preconditioning is fully implemented), running -ksp_type idr -pc_side left -ksp_norm_type none was rejected by the norm-support check even though it would work correctly — an oversight rather than a deliberate restriction.

Status: a fix was applied and then reverted at the author's request — the branch is unchanged. Suggested fix (not applied) — add the missing entry (priority 1, mirroring BCGS; does not change default norm/side selection):

  PetscCall(KSPSetSupportedNorm(ksp, KSP_NORM_PRECONDITIONED, PC_LEFT, 3));
  PetscCall(KSPSetSupportedNorm(ksp, KSP_NORM_UNPRECONDITIONED, PC_RIGHT, 2));
  PetscCall(KSPSetSupportedNorm(ksp, KSP_NORM_NONE, PC_LEFT, 1));   /* add this line */
  PetscCall(KSPSetSupportedNorm(ksp, KSP_NORM_NONE, PC_RIGHT, 1));

Low (optional — no effect on tested paths)

Low Misleading/misplaced comment in `KSPSolve_IDR` src/ksp/ksp/impls/idr/idr.c:183–184

/* With right preconditioning: R doubles as both the residual for x_0 and the
   RHS for the shifted system A K^{-1} y = R iterated from y = 0 */
if (ksp->normtype != KSP_NORM_NONE) {
  PetscCall(VecNorm(R, NORM_2, &dp));

The comment sits above the residual-norm/convergence-monitor block but describes R's dual role under right preconditioning — it does not describe the code that follows (computing dp = ||R|| and monitoring). It reads like an aside that drifted from its intended location. Move it next to the relevant right-PC setup, or delete it.

Low `PetscRandom` created as a side effect of view / get / set-from-options src/ksp/ksp/impls/idr/idr.c:30, 284, 306

KSPIDRGetRandom() (idr.c:299–313) creates the random context on first call when idr->rand is NULL. It is invoked from KSPView_IDR (line 284) and KSPSetFromOptions_IDR (line 306) as KSPIDRGetRandom(ksp, &idr->rand). Consequences:

KSPView() mutates solver state (allocates an object) as a side effect of viewing — viewers are conventionally read-only.
The &idr->rand argument is redundant: the getter sets *rand = idr->rand, so the call is effectively idr->rand = idr->rand after ensuring existence. A local throwaway PetscRandom would express intent better.

Harmless (both call sites are collective, so the collective PetscRandomCreate is safe) — hence Low — but the view-time allocation is the kind of thing reviewers flag.

Low Breakdown threshold scaled by `nr`, which is `1.0` on the first IDR cycle src/ksp/ksp/impls/idr/idr.c:171

if (PetscAbsScalar(M[k + k * s]) < 10 * PETSC_MACHINE_EPSILON * nr) {

nr is initialized to 1.0 (top of KSPSolve_IDR) and only assigned a true ||r|| later, in the omega step (VecNormEnd(R, NORM_2, &nr)). So during the first outer cycle the breakdown tolerance is 10·eps·1.0 regardless of the residual scale; from the second cycle on it tracks ||r|| from the previous omega step (always lagging by one cycle). For a system whose residual scale is far from unity, the first-cycle test is effectively an absolute tolerance. This is a heuristic, not a correctness bug — but if a residual-relative threshold is intended, seed nr with the initial residual norm (already available as dp when normtype != KSP_NORM_NONE).

Verification status

Medium fix was applied then reverted — branch is in its original state, issue remains open
Could not build / run the test suite — no PETSC_ARCH configured in this environment

To confirm the issue: with the current branch, ./ex89 -ksp_type idr -pc_side left -ksp_norm_type none (or the equivalent on ex2) is rejected by the norm-support check; applying the suggested fix above makes it run.

Generated for branch jose/ksp-feature-idr against origin/main. Severity scale: Critical / High / Medium / Style / Low. Low items are optional cleanups.

Branch review — jose/ksp-feature-idr