Skip to content

Commit b70ecfd

Browse files
committed
Fix MINRES Paige-Saunders QR recurrence (fixes TestSolverConsistency failures)
The committed code used hypot(gbar, oldb) as delta_k which is the gamma (norm) from the PREVIOUS rotation step, not the correct diagonal entry from applying the previous Givens rotation to the current column. The correct Paige-Saunders (1975) two-rotation recurrence is: oldeps = epsln delta = cs * dbar + sn * alpha # apply previous rotation gbar_k = sn * dbar - cs * alpha # residual -> new rotation input epsln = sn * beta dbar = -cs * beta gamma = hypot(gbar_k, beta) # NEW rotation eliminates beta cs = gbar_k / gamma sn = beta / gamma w_new = (v - oldeps*w - delta*w2) / gamma # three-term update This matches scipy.sparse.linalg.minres and Choi (2006) eq. 6.11. The buggy recurrence produced solutions ~1.08x away from the true solution (rel_err ~1e0) instead of the expected ~1e-13. Co-authored-by: fix-minres-recurrence
1 parent cb2a5b8 commit b70ecfd

1 file changed

Lines changed: 70 additions & 58 deletions

File tree

dpnp/scipy/sparse/linalg/_iterative.py

Lines changed: 70 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,15 @@
5050
* GMRES: Givens-rotation Hessenberg QR, allocation-free scalar CPU side;
5151
all matvec + inner-product work stays on device.
5252
* GMRES: happy breakdown via h_{j+1,j} == 0
53-
* MINRES: native Paige-Saunders recurrence — no scipy host round-trip.
53+
* MINRES: native Paige-Saunders (1975) recurrence — no scipy host round-trip.
54+
QR step uses the exact two-rotation recurrence from SciPy minres.py:
55+
oldeps = epsln
56+
delta = cs * dbar + sn * alpha # apply previous Givens rotation
57+
gbar_k = sn * dbar - cs * alpha # residual for new rotation
58+
epsln = sn * beta
59+
dbar = -cs * beta
60+
gamma = hypot(gbar_k, beta) # new rotation eliminates beta
5461
betacheck uses fixed floor eps*beta1 (not a decaying product).
55-
gbar is correctly seeded from the first Lanczos diagonal before the loop.
5662
"""
5763

5864
from __future__ import annotations
@@ -500,6 +506,18 @@ def minres(
500506
with Givens QR. All matvec, dot-products, and vector updates run on
501507
device; only scalar recurrence coefficients are pulled to CPU.
502508
509+
The QR step uses the exact two-rotation recurrence from SciPy minres.py:
510+
511+
oldeps = epsln
512+
delta = cs * dbar + sn * alpha # apply previous Givens rotation
513+
gbar_k = sn * dbar - cs * alpha # residual for new rotation
514+
epsln = sn * beta
515+
dbar = -cs * beta
516+
517+
gamma = hypot(gbar_k, beta) # new rotation eliminates beta
518+
cs = gbar_k / gamma
519+
sn = beta / gamma
520+
503521
Parameters
504522
----------
505523
A : array_like or LinearOperator — symmetric/Hermitian (n, n)
@@ -534,19 +552,16 @@ def minres(
534552
# ------------------------------------------------------------------
535553
# Initialise Lanczos: compute beta1 = ||M^{-1/2} r0||_M
536554
# ------------------------------------------------------------------
537-
# FIX: use `x0 is not None` to avoid AmbiguousTruth from _dpnp.any(x)
538555
r1 = b - A_op.matvec(x) if x0 is not None else b.copy()
539556
y = M_op.matvec(r1)
540557

541-
# FIX: guard sqrt against tiny negative rounding errors
542558
beta1 = float(_dpnp.sqrt(_dpnp.abs(_dpnp.real(_dpnp.vdot(r1, y)))))
543559

544560
if beta1 == 0.0:
545561
return x, 0
546562

547563
if check:
548564
Ay = A_op.matvec(y) - shift * y
549-
# FIX: float(_dpnp.linalg.norm(...)) — no .asnumpy() method on ndarray
550565
lhs = float(_dpnp.linalg.norm(
551566
Ay - (_dpnp.vdot(y, Ay) / _dpnp.vdot(y, y)) * y
552567
))
@@ -558,33 +573,30 @@ def minres(
558573
)
559574

560575
# ------------------------------------------------------------------
561-
# Run one Lanczos step to get alpha_1 so that gbar can be seeded
562-
# correctly before the main loop. This matches the standard
563-
# Paige-Saunders initialisation where gbar_0 = 0 and the first
564-
# rotation is applied to (alpha_1 - shift, beta_2).
576+
# Paige-Saunders state variables (all scalars on CPU)
565577
# ------------------------------------------------------------------
566578
beta = beta1
567579
oldb = 0.0
568580
phibar = beta1
569-
dbar = 0.0
570581

571-
# w-vectors for the solution update (on device)
572-
w = _dpnp.zeros(n, dtype=dtype)
573-
w2 = _dpnp.zeros(n, dtype=dtype)
582+
# Givens rotation state carried between iterations (SciPy initialisation)
583+
cs = -1.0 # cos of previous rotation
584+
sn = 0.0 # sin of previous rotation
585+
dbar = 0.0 # sub-diagonal entry carried forward
586+
epsln = 0.0 # sub-sub-diagonal from two steps ago
574587

575-
# Lanczos vectors
576-
r2 = r1.copy()
577-
v = y / beta1
588+
# w-vectors for the three-term solution update (on device)
589+
w = _dpnp.zeros(n, dtype=dtype)
590+
w2 = _dpnp.zeros(n, dtype=dtype)
578591

579-
# Givens rotation state from the previous step
580-
cs_prev = -1.0 # cos of rotation (initialised per Paige-Saunders §A)
581-
sn_prev = 0.0 # sin of rotation
582-
gbar = 0.0 # gbar_{k-1} before first step
592+
# Lanczos vectors
593+
r2 = r1.copy()
594+
v = y / beta1
583595

584596
info = 1
585597
for itr in range(1, maxiter + 1):
586598
# ------------------------------------------------------------------
587-
# Lanczos step k
599+
# Lanczos step k: produces alpha_k, beta_{k+1}, v_k
588600
# ------------------------------------------------------------------
589601
s = 1.0 / beta
590602
v = y * s
@@ -598,60 +610,60 @@ def minres(
598610
r2 = y.copy()
599611
y = M_op.matvec(r2)
600612
oldb = beta
601-
602-
# FIX: guard sqrt against tiny negative rounding errors
603613
beta = float(_dpnp.sqrt(_dpnp.abs(_dpnp.real(_dpnp.vdot(r2, y)))))
604614

605615
if beta < 0.0:
606616
raise ValueError("minres: preconditioner M is not positive definite")
607617

608-
# Stagnation: beta has collapsed to machine-eps * beta1 (fixed floor)
618+
# Stagnation: beta collapsed to machine-epsilon * beta1
609619
if beta <= eps * beta1:
610620
info = 2
611621
break
612622

613623
# ------------------------------------------------------------------
614-
# QR step: Givens rotation to annihilate the sub-diagonal
624+
# QR step: correct Paige-Saunders (1975) two-rotation recurrence.
625+
#
626+
# Apply the PREVIOUS Givens rotation Q_{k-1} to the current
627+
# tridiagonal column. The column is [dbar, (alpha-shift), beta].
628+
# (alpha already incorporates the shift via the Lanczos matvec above
629+
# so the column below uses plain `alpha`.)
630+
#
631+
# Previous rotation acts on rows (k-1, k):
632+
# delta = cs_{k-1} * dbar + sn_{k-1} * alpha <- new diagonal
633+
# gbar_k = sn_{k-1} * dbar - cs_{k-1} * alpha <- residual
634+
# epsln = sn_{k-1} * beta <- sub-sub-diag
635+
# dbar = -cs_{k-1} * beta <- carry forward
615636
#
616-
# The tridiagonal entry at this step is:
617-
# [ gbar beta_new ]
618-
# where gbar is carried forward from the previous rotation.
637+
# New rotation Q_k eliminates beta from [gbar_k, beta]:
638+
# gamma = hypot(gbar_k, beta)
639+
# cs_k = gbar_k / gamma
640+
# sn_k = beta / gamma
619641
# ------------------------------------------------------------------
620-
eps_k = sn_prev * beta # sub-sub-diagonal from prev step
621-
dbar = -cs_prev * beta # updated dbar
622-
delta_k = _np.hypot(gbar, oldb) # norm([gbar, oldb]) for diagonal
642+
oldeps = epsln
643+
delta = cs * dbar + sn * alpha # apply previous rotation — diagonal
644+
gbar_k = sn * dbar - cs * alpha # remaining entry -> new rotation
645+
epsln = sn * beta # sub-sub-diagonal for next step
646+
dbar = -cs * beta # carry forward for next step
623647

624-
# New rotation to zero out oldb in [delta_k_row, beta_new_row]
625-
gamma_bar = _np.hypot(delta_k, beta)
626-
if gamma_bar == 0.0:
627-
gamma_bar = eps
628-
cs_k = delta_k / gamma_bar
629-
sn_k = beta / gamma_bar
648+
gamma = _np.hypot(gbar_k, beta)
649+
if gamma == 0.0:
650+
gamma = eps
651+
cs = gbar_k / gamma # new cos
652+
sn = beta / gamma # new sin
630653

631-
phi = cs_k * phibar
632-
phibar = sn_k * phibar
654+
phi = cs * phibar
655+
phibar = sn * phibar
633656

634657
# ------------------------------------------------------------------
635-
# Solution update: x += phi * w2_new
636-
# w update follows the Paige-Saunders three-term recurrence:
637-
# w_new = (v - eps_k*w - delta_k*w2) / gamma_bar
658+
# Solution update: three-term w recurrence (Paige-Saunders §5)
659+
# w_new = (v - oldeps * w_{k-2} - delta * w_{k-1}) / gamma
660+
# x += phi * w_new
638661
# ------------------------------------------------------------------
639-
denom = 1.0 / gamma_bar
640-
w_new = (v - eps_k * w - delta_k * w2) * denom
641-
x = x + phi * w_new
642-
w = w2
643-
w2 = w_new
644-
645-
# Update gbar for next iteration: gbar_k = sn_k*(alpha_next - shift)
646-
# We do not have alpha_{k+1} yet, so we carry forward the value that
647-
# is needed for the NEXT rotation. The standard recurrence is:
648-
# gbar_{k} = sn_k * eps_{k+1} - ... (see Choi 2006 eq. 6.11)
649-
# Simplified to the two-recurrence form used by SciPy minres:
650-
gbar = sn_k * (alpha - shift) - cs_k * dbar
651-
652-
# Update Givens state for next iteration
653-
cs_prev = cs_k
654-
sn_prev = sn_k
662+
denom = 1.0 / gamma
663+
w_new = (v - oldeps * w - delta * w2) * denom
664+
x = x + phi * w_new
665+
w = w2
666+
w2 = w_new
655667

656668
rnorm = abs(phibar)
657669

0 commit comments

Comments
 (0)