DEP: switch sparse methods to kwarg-only; remove tol/restrt kwargs #20498

h-vetinari · 2024-04-17T00:31:39Z

Follow-up to:

…s gone

h-vetinari · 2024-04-17T02:21:26Z

scipy/sparse/linalg/_isolve/iterative.py

+def bicgstab(A, b, x0=None, *, rtol=1e-5, atol=0., maxiter=None, M=None,
+             callback=None):


Note that x0 was falsely marked as kwarg-only here; it was missed by 6476f8a

... as well as more complex warning specifications, e.g. `ignore:match_msg` or `ignore::DeprecationWarning`.

h-vetinari · 2024-04-17T05:24:34Z

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

@@ -621,6 +633,7 @@ def test_basic(self):

        assert_allclose(x_gm[0], 0.359, rtol=1e-2)

+    @pytest.mark.filterwarnings(f"ignore:{CB_TYPE_FILTER}:DeprecationWarning")


This innocent-looking line breaks our test_warnings infrastructure; I've fixed it to the best of my (limited) understanding of AST processing, but adding @rgommers / @andyfaff for opinions/help in reviewing scipy/_lib/tests/test_warnings.py.

h-vetinari · 2024-04-17T05:25:42Z

scipy/_lib/tests/test_warnings.py

+            match node.args[0]:
+                case ast.Constant() as c:


Since we dropped 3.9 in #20373, I'm assuming that match: statements are now fair game.

h-vetinari · 2024-04-17T06:24:57Z

scipy/_lib/tests/test_warnings.py

+                case _:
+                    raise ValueError("unknown ast node type")
+            # check if filter is set to ignore
+            if argtext.split(":")[0] == "ignore":


Guess I shouldn't have taught the parser here to be smarter about finding ignored warnings. It finds quite a few of them now 😅

Probably worth splitting this off into a separate PR...

h-vetinari · 2024-04-17T06:28:54Z

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

@@ -724,6 +737,7 @@ def test_defective_matrix_breakdown(self):
        # The solution should be OK outside null space of A
        assert_allclose(A @ (A @ x), A @ b)

+    @pytest.mark.filterwarnings(f"ignore:{CB_TYPE_FILTER}:DeprecationWarning")


OTOH, I find this line much nicer than (as the failing test tells me to) indenting almost the whole test another level to wrap it in numpy.testing.suppress_warnings (resp. pytest.warns), or adding it 4x for the individual gmres calls.

h-vetinari · 2024-04-20T08:22:32Z

I see the mouse-over text for needs-work Items that are pending response from the author includes the reviewers, but that to me is not clear at all. It seems squarely aimed at the PR author. Can we not distinguish these into "needs work" and "needs review"?

lucascolley · 2024-04-20T09:38:20Z

I see the mouse-over text for needs-work includes the reviewers, but that to me is not clear at all. It seems squarely aimed at the PR author. Can we not distinguish these into "needs work" and "needs review"?

I've thought about this a bit while triaging first-time contributor PRs. needs-review is really just the absence of needs-work or needs-decision (or needs-champion). The main utility of these labels is for PRs that have been open for a while IMO - there would be no point adding and removing them if the PR is only going to be open a few days.

I wouldn't be opposed to a needs-review, but I think there is a decent argument that it would just add clutter (given that really, if a PR is open, a review would nearly always be a good thing - if it wouldn't, that's what draft PRs are for).

So I propose we change the mouse-over text to just refer to the PR author.

ilayn · 2024-04-20T09:47:36Z

This PR makes me really happy. Decades-old crust is finally going away. Thanks all for the hard work!

j-bowhay

The deprecation changes look good but could we split the changes in scipy/_lib/tests/test_warnings.py off so we don't have to deal with the test failures in this pr

h-vetinari · 2024-04-20T11:56:24Z

could we split the changes in scipy/_lib/tests/test_warnings.py off

No, see here

so we don't have to deal with the test failures in this pr

That OTOH is possible.

h-vetinari · 2024-04-20T23:23:14Z

This PR makes me really happy. Decades-old crust is finally going away. Thanks all for the hard work!

Thanks for the kind words, though changing a couple function signatures is a far cry from the main work you did in #18488! 💪

ilayn · 2024-04-21T06:20:18Z

though changing a couple function signatures is a far cry

Yes indeed that was not fun either, but meticulously tracking items over versions is not a trivial feat at all. One comment we are removing in this PR says "Handle deprecation frenzy", and frenzy it is.

Thus, much appreciated 😉

j-bowhay

Thanks @h-vetinari and everyone else

lucascolley · 2024-04-21T12:34:40Z

So I propose we change the mouse-over text to just refer to the PR author.

I changed this.

h-vetinari added scipy.sparse.linalg deprecated Items related to behavior that has been deprecated labels Apr 17, 2024

h-vetinari requested a review from perimosocordiae as a code owner April 17, 2024 00:31

github-actions bot added the scipy.sparse label Apr 17, 2024

h-vetinari force-pushed the isolve_tol_kwargs branch from ce87c83 to e7d8579 Compare April 17, 2024 00:40

h-vetinari mentioned this pull request Apr 17, 2024

META: deprecation tracker #15765

Open

32 tasks

h-vetinari force-pushed the isolve_tol_kwargs branch 2 times, most recently from fae34bd to 19e66ab Compare April 17, 2024 01:40

h-vetinari added 2 commits April 17, 2024 13:03

DEP: switch sparse methods to kwarg-only; remove tol/restart kwargs

ced1a7d

MAINT: reflect gmres deprecation, now that too-broad warning filter i…

5f192c6

…s gone

h-vetinari force-pushed the isolve_tol_kwargs branch from 19e66ab to 5f192c6 Compare April 17, 2024 02:05

h-vetinari commented Apr 17, 2024

View reviewed changes

TST: teach test_warnings infrastructure to handle f-strings

7c02842

... as well as more complex warning specifications, e.g. `ignore:match_msg` or `ignore::DeprecationWarning`.

h-vetinari requested review from rgommers and andyfaff April 17, 2024 05:22

h-vetinari commented Apr 17, 2024

View reviewed changes

h-vetinari changed the title ~~DEP: switch sparse methods to kwarg-only; remove tol/restart kwargs~~ DEP: switch sparse methods to kwarg-only; remove tol/restrt kwargs Apr 17, 2024

h-vetinari commented Apr 17, 2024

View reviewed changes

j-bowhay added the needs-work Items that are pending response from the author label Apr 19, 2024

j-bowhay reviewed Apr 20, 2024

View reviewed changes

don't improve detection of ignored tests just yet

010b16c

j-bowhay approved these changes Apr 21, 2024

View reviewed changes

j-bowhay added this to the 1.14.0 milestone Apr 21, 2024

j-bowhay merged commit 0b94951 into scipy:main Apr 21, 2024

h-vetinari deleted the isolve_tol_kwargs branch April 21, 2024 12:18

lucascolley removed the needs-work Items that are pending response from the author label Apr 21, 2024

h-vetinari mentioned this pull request May 26, 2024

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

Draft

lucascolley mentioned this pull request Aug 5, 2024

DOC: optimize.root: fix docs for inner_* parameters #21296

Closed

h-vetinari mentioned this pull request Dec 23, 2024

TST: optimize: suppress incorrect sparray warning from scikit-sparse #22089

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

DEP: switch sparse methods to kwarg-only; remove tol/restrt kwargs #20498

DEP: switch sparse methods to kwarg-only; remove tol/restrt kwargs #20498

Uh oh!

h-vetinari commented Apr 17, 2024

Uh oh!

h-vetinari Apr 17, 2024

Uh oh!

h-vetinari Apr 17, 2024

Uh oh!

h-vetinari Apr 17, 2024

Uh oh!

h-vetinari Apr 17, 2024

Uh oh!

h-vetinari Apr 17, 2024 •

edited

Loading

Uh oh!

h-vetinari commented Apr 20, 2024

Uh oh!

lucascolley commented Apr 20, 2024 •

edited

Loading

Uh oh!

ilayn commented Apr 20, 2024

Uh oh!

j-bowhay left a comment •

edited

Loading

Uh oh!

h-vetinari commented Apr 20, 2024

Uh oh!

h-vetinari commented Apr 20, 2024

Uh oh!

ilayn commented Apr 21, 2024

Uh oh!

j-bowhay left a comment

Uh oh!

lucascolley commented Apr 21, 2024

Uh oh!

Uh oh!

		def bicgstab(A, b, x0=None, *, rtol=1e-5, atol=0., maxiter=None, M=None,
		callback=None):

		@@ -621,6 +633,7 @@ def test_basic(self):

		assert_allclose(x_gm[0], 0.359, rtol=1e-2)

		@pytest.mark.filterwarnings(f"ignore:{CB_TYPE_FILTER}:DeprecationWarning")

Uh oh!

DEP: switch sparse methods to kwarg-only; remove tol/restrt kwargs #20498

DEP: switch sparse methods to kwarg-only; remove tol/restrt kwargs #20498

Uh oh!

Conversation

h-vetinari commented Apr 17, 2024

Uh oh!

h-vetinari Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

h-vetinari Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

h-vetinari Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

h-vetinari Apr 17, 2024

Choose a reason for hiding this comment

Uh oh!

h-vetinari Apr 17, 2024 • 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!

h-vetinari commented Apr 20, 2024

Uh oh!

lucascolley commented Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilayn commented Apr 20, 2024

Uh oh!

j-bowhay left a comment • 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!

h-vetinari commented Apr 20, 2024

Uh oh!

h-vetinari commented Apr 20, 2024

Uh oh!

ilayn commented Apr 21, 2024

Uh oh!

j-bowhay left a comment

Choose a reason for hiding this comment

Uh oh!

lucascolley commented Apr 21, 2024

Uh oh!

Uh oh!

h-vetinari Apr 17, 2024 •

edited

Loading

lucascolley commented Apr 20, 2024 •

edited

Loading

j-bowhay left a comment •

edited

Loading