DEP: sparse.linalg: remove msg on `gcrotmk`, add `atol` default #16050

jvsqzj · 2022-04-27T01:41:03Z

Reference issue

What does this implement/fix?

Successfully removes all deprecated default parameter warning messages from functions in _isolve
Docstrings updated to match method behavior and usage

Additional information

h-vetinari · 2022-04-27T02:27:01Z

Closes #16050

Should be #15738

Still missing a whole bunch of instances in sparse/linalg/_isolve/ as discussed in the issue.

h-vetinari · 2022-04-27T02:28:22Z

scipy/sparse/linalg/_isolve/_gcrotmk.py

-        warnings.warn("scipy.sparse.linalg.gcrotmk called without specifying `atol`. "
-                      "The default value will change in the future. To preserve "
-                      "current behavior, set ``atol=tol``.",
-                      category=DeprecationWarning, stacklevel=2)
        atol = tol


This should now raise an error, not set atol = tol.

h-vetinari · 2022-04-27T02:33:52Z

scipy/sparse/linalg/_isolve/_gcrotmk.py

+
+    if type(atol) != float:
+        raise ValueError("Tolerance value expected float")


It should be possible to replace both if-conditions with

Suggested change

if type(atol) != float:

raise ValueError("Tolerance value expected float")

if not np.issubdtype(type(atol), np.floating):

raise ValueError("Parameter `atol` must be a floating-point number!")

jvsqzj · 2022-04-28T04:46:45Z

scipy/sparse/linalg/_isolve/iterative.py

-        else:
-            return tol * float(bnrm2)
-    else:
-        return max(float(atol), tol * float(bnrm2))


@h-vetinari I assume we want to remove this legacy behavior here. This _get_atol()function becomes rather small after removing the legacy code.

Also, about scipy/sparse/linalg/_isolve/iterative.py:576

if callback is not None and callback_type is None: # Warn about 'callback_type' semantic changes. # Probably should be removed only in far future, Scipy 2.0 or so. warnings.warn("scipy.sparse.linalg.gmres called without specifying `callback_type`. " "The default value will be changed in a future release. " "For compatibility, specify a value for `callback_type` explicitly, e.g., " "``{name}(..., callback_type='pr_norm')``, or to retain the old behavior " "``{name}(..., callback_type='legacy')``", category=DeprecationWarning, stacklevel=3) if callback_type is None: callback_type = 'legacy'

Do we also want this warning gone? I suppose its better deprecated now than later.

@h-vetinari I assume we want to remove this legacy behavior here. This _get_atol()function becomes rather small after removing the legacy code.

Indeed, that function only exists to work around the deprecation, and we should IMO remove it completely. You'll need to make sure that at each call site, the maximum of atol and the correct(ly calculated) residual is taken.

Also, about scipy/sparse/linalg/_isolve/iterative.py:576
[...]
Do we also want this warning gone? I suppose its better deprecated now than later.

It's deprecated already, and a whole separate issue IMO. I suggest not to touch it in this PR.

Alright. I think I'm done with gcrotmk and lgmres and their corresponding test files. However, I've stumbled upon issues when trying to remove the deprecation messages from iterative.py and test_iterative.py because I see some functions receive solver as argument for different functions i.e:

# list of tuples (solver, symmetric, positive_definite ) solvers = [cg, cgs, bicg, bicgstab, gmres, qmr, minres, lgmres, gcrotmk, tfqmr] sym_solvers = [minres, cg] posdef_solvers = [cg] real_solvers = [minres]

So, I'm having some conflicts due to some of these solvers not even having the atol parameter to start with. I suppose this is the reason _get_atol() was implemented in the first place.

tylerjereddy · 2022-11-26T23:45:50Z

Since CI is failing several tests, and the review process doesn't quite look complete, and deprecation-related cleanups probably aren't urgent (though can be quite nice) for releases, I'll likely bump the milestone to keep my "checklist" concise with branching scheduled in ~10 days.

If this PR suddenly kicks back into action and gets merged on time, of course you can set the milestone back after merging.

j-bowhay · 2023-05-20T07:41:46Z

We should hold fire on this and reevaluate after #18488

h-vetinari · 2023-05-20T07:46:42Z

We should hold fire on this and reevaluate after #18488

100%, that was my intention as well. I just bumped the milestone to wind down the amount of stuff in there which is going to miss 1.11 anyway.

h-vetinari · 2023-05-20T07:48:06Z

And to not disparage the work of @jvsqzj, this is a really thorny subject that's been unsolved for many years, so please don't feel bad about things taking longer and turning out differently! :)

tylerjereddy · 2023-12-04T21:27:31Z

@h-vetinari not sure what is going on here, but the matching issue is now closed even though some of the warnings removed in this PR do remain in our latest source.

In any case, I'm not comfortable merging this due to the tricky API change and the merge conflicts that have shown up over the months. I'll bump the milestone, but since 1.13.0 may come out a few months earlier than usual (because of NumPy 2.0.0 support), there will likely be another medium-term opportunity to deal with this if desired.

j-bowhay · 2023-12-04T22:22:14Z

I think we can close this as redundant as of #18488 regardless thanks for your contribution @jvsqzj

ilayn · 2023-12-04T22:31:35Z

#18488 did not touch gcrotmk and lmgres so I think this is still a valid PR. Apologies not being precise, I'm too invested in special fortran code right now so don't remember all the details about what we did for this.

h-vetinari · 2023-12-05T03:46:48Z

#18488 did not touch gcrotmk and lmgres so I think this is still a valid PR.

Sounds like we need to reopen #15738 too?

h-vetinari · 2023-12-13T02:12:38Z

Does someone (@jvsqzj?) want to merge main here and see where things stand?

h-vetinari · 2023-12-17T23:57:53Z

I merged main here, but looking closer, I actually don't think we want to do this (switch to errors directly), at least not yet. I'll open another PR.

lucascolley · 2023-12-19T13:05:37Z

I actually don't think we want to do this (switch to errors directly), at least not yet

Do we want to leave this PR open or close it now?

h-vetinari · 2023-12-19T13:22:21Z

Given that it's @jvsqzj's first contribution, I'm inclined to make an exception and leave it open. It will become relevant for 1.14, which is only ~6 month away, and the PR has already been open for 1.5 years, so this shouldn't make a big difference. I can rebase it once 1.13 branches (unless someone beats me to it).

lucascolley · 2024-03-18T15:24:25Z

Given that it's @jvsqzj's first contribution, I'm inclined to make an exception and leave it open. It will become relevant for 1.14, which is only ~6 month away, and the PR has already been open for 1.5 years, so this shouldn't make a big difference. I can rebase it once 1.13 branches (unless someone beats me to it).

1.13 has branched so just a reminder here @h-vetinari :)

h-vetinari · 2024-03-19T00:02:22Z

@jvsqzj, are you still here? 🙃

Would you like to keep working on this PR, or should we take it from here?

jvsqzj · 2024-03-19T16:20:31Z

@jvsqzj, are you still here? 🙃

Would you like to keep working on this PR, or should we take it from here?

I would like to. I'm sorry, work consuming all my time usually.
Need to set some time to continue this.

tylerjereddy · 2024-05-26T22:54:25Z

It will become relevant for 1.14,

@h-vetinari can you provide a concise explanation of why we need to worry about this PR for 1.14.0 release? Deprecation timing-related stuff or?

h-vetinari · 2024-05-26T23:31:18Z

I think #20498 did everything that's necessary from the POV of executing deprecations. I think this PR is still useful because it contains a couple more test clean-ups that I had missed, but shouldn't be critical

lucascolley · 2024-11-30T21:54:07Z

@jvsqzj just another ping in case you are interested in returning to this!

h-vetinari reviewed Apr 27, 2022

View reviewed changes

tupui added deprecated Items related to behavior that has been deprecated scipy.sparse.linalg labels Apr 27, 2022

jvsqzj force-pushed the DEP-default-atol-15738 branch from 7b4b59d to e716dad Compare April 28, 2022 04:44

jvsqzj commented Apr 28, 2022

View reviewed changes

h-vetinari mentioned this pull request Apr 28, 2022

DEP: change default of atol in scipy.sparse.linalg.* #15738

Closed

Juan Vasquez added 2 commits May 1, 2022 11:50

Remove default atol from lgmres

5dc9d9d

Set explicit atol value for gcrotmk tests

2c229aa

jvsqzj force-pushed the DEP-default-atol-15738 branch from e716dad to 2c229aa Compare May 2, 2022 00:04

Change atol to float type on arpack

20d478d

j-bowhay added this to the 1.10.0 milestone Jul 8, 2022

tylerjereddy modified the milestones: 1.10.0, 1.11.0 Nov 26, 2022

ilayn mentioned this pull request May 1, 2023

MAINT:ENH:sparse.linalg: Rewrite iterative solvers in Python, remove FORTRAN code #18391

Closed

ilayn mentioned this pull request May 18, 2023

MAINT:ENH:sparse.linalg: Rewrite iterative solvers in Python #18488

Merged

h-vetinari modified the milestones: 1.11.0, 1.12.0 May 20, 2023

tylerjereddy modified the milestones: 1.12.0, 1.13.0 Dec 4, 2023

j-bowhay closed this Dec 4, 2023

j-bowhay removed this from the 1.13.0 milestone Dec 4, 2023

j-bowhay reopened this Dec 4, 2023

lucascolley added this to the 1.13.0 milestone Dec 10, 2023

Merge remote-tracking branch 'upstream/main' into DEP-default-atol-15738

3363ba5

h-vetinari mentioned this pull request Dec 18, 2023

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

Merged

lucascolley removed this from the 1.13.0 milestone Dec 19, 2023

lucascolley added this to the 1.14.0 milestone Mar 12, 2024

lucascolley self-requested a review March 12, 2024 20:23

lucascolley changed the title ~~WIP remove msg on gcrotmk, add atol default~~ DEP: sprase.linalg: remove msg on gcrotmk, add atol default Mar 14, 2024

lucascolley changed the title ~~DEP: sprase.linalg: remove msg on gcrotmk, add atol default~~ DEP: sparse.linalg: remove msg on gcrotmk, add atol default Mar 14, 2024

lucascolley removed their request for review March 18, 2024 15:24

tylerjereddy modified the milestones: 1.14.0, 1.15.0 May 26, 2024

lucascolley removed this from the 1.15.0 milestone Nov 30, 2024


		if type(atol) != float:
		raise ValueError("Tolerance value expected float")

Uh oh!

DEP: sparse.linalg: remove msg on gcrotmk, add atol default #16050

Are you sure you want to change the base?

DEP: sparse.linalg: remove msg on gcrotmk, add atol default #16050

Uh oh!

Conversation

jvsqzj commented Apr 27, 2022 • edited by tupui Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference issue

What does this implement/fix?

Additional information

Uh oh!

h-vetinari commented Apr 27, 2022

Uh oh!

h-vetinari Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

h-vetinari Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

jvsqzj Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvsqzj Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

h-vetinari Apr 28, 2022

Choose a reason for hiding this comment

Uh oh!

jvsqzj May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerjereddy commented Nov 26, 2022

Uh oh!

j-bowhay commented May 20, 2023

Uh oh!

h-vetinari commented May 20, 2023

Uh oh!

h-vetinari commented May 20, 2023

Uh oh!

tylerjereddy commented Dec 4, 2023

Uh oh!

j-bowhay commented Dec 4, 2023

Uh oh!

ilayn commented Dec 4, 2023

Uh oh!

h-vetinari commented Dec 5, 2023

Uh oh!

h-vetinari commented Dec 13, 2023

Uh oh!

h-vetinari commented Dec 17, 2023

Uh oh!

lucascolley commented Dec 19, 2023

Uh oh!

h-vetinari commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucascolley commented Mar 18, 2024

Uh oh!

h-vetinari commented Mar 19, 2024

Uh oh!

jvsqzj commented Mar 19, 2024

Uh oh!

tylerjereddy commented May 26, 2024

Uh oh!

h-vetinari commented May 26, 2024

Uh oh!

lucascolley commented Nov 30, 2024

Uh oh!

Uh oh!

DEP: sparse.linalg: remove msg on `gcrotmk`, add `atol` default #16050

DEP: sparse.linalg: remove msg on `gcrotmk`, add `atol` default #16050

jvsqzj commented Apr 27, 2022 •

edited by tupui

Loading

jvsqzj Apr 28, 2022 •

edited

Loading

jvsqzj May 2, 2022 •

edited

Loading

h-vetinari commented Dec 19, 2023 •

edited

Loading