DEP: Adopt *tol deprecations also for gcrotmk/lgmres/minres/tfqmr

[h-vetinari]

While #18488 solved most of #15738, it left out gcrotmk / lgmres / minres / tfqmr, which are defined differently (in python directly).

Make the deprecation work around tol/atol/rtol uniform for those functions, which otherwise have the same interface.

Marked as a backport because IMO the current status is quite broken (including some of the warning text, see first commit).

This is an alternative to #16050, which does a more aggressive switch (requiring float atol directly) that we didn't do for the other scipy.sparse.linalg.* functions.

Closes #15738

[h-vetinari]

@ilayn @j-bowhay @tupui @mdhaber @andyfaff @lucascolley
This is ready. It turned out a bit bigger than I hoped for, but I'd very much appreciate if someone could review soon, because IMO we shouldn't release the current state of affairs in 1.12, and so I'd like to get this in ASAP to still have a chance to backport it.

I'm fine with squash-merging this for the convenience of the backport, but just a quick explanation of the commits:

• c0201a8 - clean up pretty bad state of warnings (introduced in 1.12)
• 7ab030c - what this PR started out as (following this comment)
• e266e33 - non-essential but benign
• 5ae5901 - I have no idea how scipy/sparse/linalg/_isolve/tests/test_iterative.py currently passes for all the solvers that have already deprecated tol=, but in any case, things fail badly without this
• 5715dd6 - necessary preparation for the following commit, and also an improvement because:
  • we avoid redundant calculations
  • we don't swallow the warnings about the *tol kwargs if norm(b) == 0
• 6e612d3 - make other remaining solvers in scipy.sparse.linalg.* consistent w.r.t. *tol handling (the only outstanding ones being lsmr and lsqr, which need deeper changes)
• 6937e3b - latent inaccuracy exposed by the float() cast in _get_atol_rtol; rho is mathematically real, but the way it used to be calculated always yielded a +0j.

FYI @tylerjereddy

[lucascolley]

Diff looks good after a quick look through. I'll have a more detailed look later.

[lucascolley]

I have no idea how scipy/sparse/linalg/_isolve/tests/test_iterative.py currently passes for all the solvers that have already deprecated tol=

@ilayn added

scipy/scipy/sparse/linalg/_isolve/tests/test_iterative.py

Line 30 in c4ce0c4

pytest.mark.filterwarnings("ignore:.* keyword argument 'tol'.*"),

Your first commit changes from quote marks to backticks around tol in the warning message, hence introduces all of the failures :)

[lucascolley]

Looks great overall! Not familiar with this part of the codebase but the diff and your explanations make sense.

See the comment above this and the following. Non-blocking nits inline.

---

scipy/scipy/sparse/linalg/_isolve/tests/test_iterative.py

Lines 373 to 379 in c4ce0c4

	# Historically this is tested as below, all pass but for some reason
	# gcrotmk is over-sensitive to difference between random.seed/rng.random
	# Hence tol lower bound is changed from -10 to -9
	# np.random.seed(1234)
	# A = np.random.rand(10, 10)
	# A = A @ A.T + 10 * np.eye(10)
	# b = 1e3*np.random.rand(10)

I don't completely understand this comment but I assume it shouldn't reference tol anymore since the rest of the function doesn't.

[h-vetinari]

Your first commit changes from quote marks to backticks around tol in the warning message, hence introduces all of the failures :)

Thanks for digging that up - I have a bit of a blindspot for blanket ignores (because I try to avoid them). IMO we should be dogfooding here and not hide those warnings, so I think we should keep 5ae5901.

[lucascolley]

Yep, makes sense to remove the blanket ignore if no longer needed.

[lucascolley]

LGTM, just the comment left from #19702 (review) but not exactly blocking 😅

[h-vetinari]

LGTM, just the comment left from #19702 (review) but not exactly blocking 😅

Yeah, I looked at this, there's nothing relevant to do here.

[tupui]

LGTM, thanks for doing that @h-vetinari and thanks for the review @lucascolley 👍

[rgommers]

This PR seems to have broken the pre-release CI job - not sure if directly or via a cross-merge conflict maybe. See e.g. this CI log from an hour ago, it has lots of failures like:

 FAILED scipy/sparse/linalg/_isolve/tests/test_iterative.py::test_positional_deprecation[bicg] - DeprecationWarning: 'scipy.sparse.linalg.bicg' keyword argument `tol` is deprecated in favor of `rtol` and will be removed in SciPy v1.14.0. Until then, if set, it will override `rtol`.
FAILED scipy/sparse/linalg/_isolve/tests/test_iterative.py::test_positional_deprecation[bicgstab] - DeprecationWarning: 'scipy.sparse.linalg.bicgstab' keyword argument `tol` is deprecated in favor of `rtol` and will be removed in SciPy v1.14.0. Until then, if set, it will override `rtol

Would you having a look at that @h-vetinari?

[lucascolley]

I'm confused as to why CI only
started failing some time after this PR, but maybe a filter needs to be added here?

https://github.com/scipy/scipy/blob/main/scipy/sparse/linalg/_isolve/tests/test_iterative.py#L554-L555

[lucascolley]

@rgommers new pytest prerelease yesterday looks like the culprit? hmm, maybe not, but all of the CI failures in that log are warnings. I wonder whether something changed in handling of nested warnings?

[rgommers]

pytest changes could well be the thing that surfaced this, good catch. That said, this PR doesn't quite look as expected. Normally when there are deprecation warnings emitted by the code, then there are some existing tests that have to be wrapped in a with warnings.catch_warnings: ... block which filters out the specific deprecation. I don't see any filters here (there may be a reason though, I'm not looking too deeply).

[lucascolley]

Aha found it

pytest-dev/pytest#9288: ~pytest.warns{.interpreted-text role="func"} now re-emits unmatched warnings when the context closes -- previously it would consume all warnings, hiding those that were not matched by the function.

While this is a new feature, we announce it as a breaking change because many test suites are configured to error-out on warnings, and will
therefore fail on the newly-re-emitted warnings.

I'll open a new issue

[h-vetinari]

Yeah, this is a case where we're warning both on positional use, as well as using tol= at all (it would be hard to merge those warnings).

It looks like the best way to deal with this IMO would be just to expand the match-pattern with a | to cover both cases.

[lucascolley]

It looks like the best way to deal with this IMO would be just to expand the match-pattern with a | to cover both cases.

I suppose the alternative is to have two separate tests, filtering out one of the warnings in each.

[h-vetinari]

I suppose the alternative is to have two separate tests, filtering out one of the warnings in each.

Aside from the syntactic overhead and code duplication, this is pointless IMO because those warnings can never be raised separately. Using positional tol= necessarily gets both (unless we do deep surgery to hand-roll the decorator, which would be even worse).

[h-vetinari]

Would you having a look at that @h-vetinari?

#19806. It's a quick fix, but should be OK for now (even for backporting; though I ask not to squash-merge that PR). Alternative would of course be to just limit to pytest <8 on the 1.12 branch.