Skip to content

Commit 80b3eea

Browse files
committed
Address review: trim comment, scipy-only test comparison, changelog
- Condense the inline comment in LSQR to a single line. - Drop the dense diag(inv(A^H A)) comparison from the test and keep scipy's LSQR as the reference; LSQR is not guaranteed to match the dense variance. - Use niter = 2 * nx (scipy's default) and note that niter is only a ceiling. - Move the issue reference out of the docstring into the targeted comment. - Add a CHANGELOG.md entry.
1 parent d98c453 commit 80b3eea

3 files changed

Lines changed: 12 additions & 16 deletions

File tree

CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@ Changelog
1717
and `pylops.waveeqprocessing.Marchenko`
1818
* Improved typing annotations across all operators (and enforced use of
1919
Literal for parameters with multiple options)
20+
* Fixed element-wise variance accumulation in `pylops.optimization.cls_basic.LSQR`
21+
and `pylops.lsqr`, which previously assigned the same value to every entry of
22+
`var` (#639)
2023

2124
# 2.6.0
2225
* Added `pylops.medical` module and `pylops.medical.CT2D` operator

pylops/optimization/cls_basic.py

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1248,11 +1248,7 @@ 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).
1251+
# accumulate the element-wise square of dk (one variance per unknown)
12561252
self.var = self.var + to_numpy_conditional(
12571253
self.var, self.ncp.square(self.dk)
12581254
)

pytests/test_solver.py

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -379,29 +379,26 @@ def test_lsqr_pylops_scipy(par):
379379

380380
@pytest.mark.parametrize("par", [(par3), (par4)])
381381
def test_lsqr_calc_var(par):
382-
"""LSQR ``var`` estimates the diagonal of (A^H A)^-1 element-wise (issue #639).
382+
"""LSQR ``var`` estimates the diagonal of (A^H A)^-1 element-wise.
383383
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.
384+
Each unknown must get its own variance; a regression would make all entries
385+
identical. scipy's LSQR is used as the reference implementation.
387386
"""
388387
np.random.seed(10)
389388

390389
A = np.random.normal(0, 1, (par["ny"], par["nx"]))
391390
Aop = MatrixMult(A, dtype=par["dtype"])
392391
y = Aop * (np.ones(par["nx"]))
393392

394-
niter = 10 * par["nx"]
393+
# niter is only a ceiling (atol/btol stop earlier); use scipy's default of
394+
# 2 * nx so both implementations run for the same number of iterations.
395+
niter = 2 * par["nx"]
395396
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))
397+
var_sp = sp_lsqr(Aop, y, iter_lim=niter, atol=1e-8, btol=1e-8, calc_var=True)[9]
400398

401-
# not all identical (the bug produced a constant vector)
399+
# the dot(dk, dk) bug produced a constant vector instead (issue #639)
402400
assert not np.allclose(var, var[0])
403401
assert_array_almost_equal(var, var_sp, decimal=6)
404-
assert_array_almost_equal(var, var_dense, decimal=6)
405402

406403

407404
@pytest.mark.parametrize(

0 commit comments

Comments
 (0)