Skip to content

Commit d98c453

Browse files
committed
Fix elementwise variance accumulation in lsqr (closes #639)
lsqr's optional `var` output estimates the diagonal of (A^H A)^-1, one variance per unknown. The update accumulated `ncp.dot(self.dk, self.dk)`, which is the scalar sum of squares of `dk`, so the same value was added to every entry of `var` and all returned variances came out identical (and wrong). Accumulate the elementwise square `ncp.square(self.dk)` instead, matching Paige & Saunders (1982, p.53) and scipy's lsqr. The solution path is unchanged; only the `var` output is corrected. Add test_lsqr_calc_var asserting the variances are not all identical and match both scipy's lsqr and the dense diag(inv(A^H A)) on full-column-rank systems.
1 parent ebe4e15 commit d98c453

2 files changed

Lines changed: 33 additions & 1 deletion

File tree

pylops/optimization/cls_basic.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1248,8 +1248,13 @@ def step(self, x: NDArray, show: bool = False) -> NDArray:
12481248
self.ncp.add(self.v, self.w, out=self.w)
12491249
self.ddnorm = self.ddnorm + self.ncp.linalg.norm(self.dk) ** 2.0
12501250
if self.calc_var:
1251+
# ``var`` estimates the diagonal of (A^H A)^-1, so it must accumulate
1252+
# the *element-wise* square of ``dk`` (one variance per unknown), as in
1253+
# Paige & Saunders (1982) eq. on p.53 and scipy's lsqr. Using
1254+
# ``dot(dk, dk)`` instead adds the same scalar (sum of squares) to every
1255+
# entry, yielding identical, wrong variances (issue #639).
12511256
self.var = self.var + to_numpy_conditional(
1252-
self.var, self.ncp.dot(self.dk, self.dk)
1257+
self.var, self.ncp.square(self.dk)
12531258
)
12541259

12551260
# use a plane rotation on the right to eliminate the

pytests/test_solver.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -377,6 +377,33 @@ def test_lsqr_pylops_scipy(par):
377377
assert_array_almost_equal(xinv_sp, x, decimal=4)
378378

379379

380+
@pytest.mark.parametrize("par", [(par3), (par4)])
381+
def test_lsqr_calc_var(par):
382+
"""LSQR ``var`` estimates the diagonal of (A^H A)^-1 element-wise (issue #639).
383+
384+
Each unknown must get its own variance; a regression here would make all
385+
entries identical (the previous ``dot(dk, dk)`` bug). Compare against scipy's
386+
LSQR and the dense ``diag(inv(A^H A))`` on a full-column-rank system.
387+
"""
388+
np.random.seed(10)
389+
390+
A = np.random.normal(0, 1, (par["ny"], par["nx"]))
391+
Aop = MatrixMult(A, dtype=par["dtype"])
392+
y = Aop * (np.ones(par["nx"]))
393+
394+
niter = 10 * par["nx"]
395+
var = lsqr(Aop, y, x0=None, niter=niter, atol=1e-8, btol=1e-8)[9]
396+
var_sp = sp_lsqr(
397+
Aop, y, iter_lim=niter, atol=1e-8, btol=1e-8, calc_var=True
398+
)[9]
399+
var_dense = np.diag(np.linalg.inv(np.conj(A.T) @ A))
400+
401+
# not all identical (the bug produced a constant vector)
402+
assert not np.allclose(var, var[0])
403+
assert_array_almost_equal(var, var_sp, decimal=6)
404+
assert_array_almost_equal(var, var_dense, decimal=6)
405+
406+
380407
@pytest.mark.parametrize(
381408
"par", [(par1), (par2), (par3), (par4), (par1j), (par2j), (par3j), (par3j)]
382409
)

0 commit comments

Comments
 (0)