Wow just wow.

I think this is close to the finished product. I'll stabilize the tests and tolerance adjustments mostly due to switching from random.seed to rng hence unearthing some unnecessarily tight tolerances that works only to those problems.

There is a GMRES issue about a premature "breakdown" but once that's fixed there are no showstoppers.

However there are many many issues, piled up overtime but not addressed, that we have to address, hopefully, with more actual users of these tools. These are mostly tolerance handling, callback strategy unification and solver performance in general such as missing stagnation checks and proper display of progress during iterations.

I am going to be just making them good enough so that we don't have new features (except deprecations) and leave it at that in this PR. My hope is that now that they are in Python more eyes can see the suboptimal parts and potentially fix them.

I would really appreciate all feedback.

Thanks a lot @h-vetinari There is indeed quite some work to do. I'll address them in the evening I hope today. In general I agree with almost everything except the commit hygiene which is exceptionally difficult given the scope.

clarify the situation w.r.t. tol= vs. rtol= also in the tests file; i.e. don't split up the calls everywhere but filter out the DeprecationWarning and add a todo to change everything from tol= to rtol=.

This one is tricky because if I start touching all solvers then it would be definitely impossible to review. The tricky part is that currently we are carrying 3(or 4 depending on how you count) different legacy ways and deprecating 2 of them simultaneously. I can touch the others but it won't make it easier to review. So at some point this has to be a mess.

• all stylistic changes (~black, rng clean-up, etc.) in the test file should be done in a separate PR IMO - that way the diff here becomes much less noisy and we could say "see, only the implementation changed"

Maybe but it won't have less clutter because there is unfortunately no such separation. Because when you switch to rng, for example, many tests start to fail. It's almost like the test precision is tailored manually. Hence it is a bit tricky.

I find it handling it easier in the split view without whitespace button clicked. But keeping track of the test results and commit separation simultaneously is a bit too much for me. I'm barely holding the logic in my head. And frankly, commit history hygiene is a bit of cargo cult now when you are changing hundreds of lines.

I don't like the duplication of the solver list all over the place, would prefer to turn it into a fixture that's auto-injected as soon as a test has solver in the signature. I've left some suggestions to implement this

Fixtures would also make things very cluttered mind you but I agree. There were apparently way too many springs that we skipped the cleaning for this module.

Also, I would consider leaving assert_, not least because PEP 679 might someday land in CPython, and we could avoid some churn. If you don't buy this argument, then I feel that assert (...) is strictly worse than assert ... because the whitespace in between now has semantic significance -- so if you want to change it, then please also remove the parentheses.

I have no opinion about this anymore, for some years we tried to get rid of assert_ through pytest adoption, now the wind is turning again probably. Both assert and assert_ are terrible in my opinion and assert_ is extra ugly with the underscore signifying the homemade wonkiness. I didn't go around and touched all those lines since it really becomes messy then. I could just rewrite all the tests.

I think the last three could be tackled in preparatory PRs. If you want, I can help with the fixture one.

Probably I can separate those PRs, only, after we are done with everything. But like I mentioned, there is no organic division that would still make tests pass. Certain things need to be touched simultaneously. I'll first get them passing to an acceptable level and then let's reconvene and assess what we can do to modernize the whole thing.

clarify the situation w.r.t. tol= vs. rtol= also in the tests file; i.e. don't split up the calls everywhere but filter out the DeprecationWarning and add a todo to change everything from tol= to rtol=.

This one is tricky because if I start touching all solvers then it would be definitely impossible to review.

My proposal was to currently leave the tests with passing tol= and filtering the deprecation warning. That way you don't have to do everything at once, but it's still clear what needs to be done (even from reading the tests).

all stylistic changes (~black, rng clean-up, etc.) in the test file should be done in a separate PR IMO - that way the diff here becomes much less noisy and we could say "see, only the implementation changed"

Maybe but it won't have less clutter because there is unfortunately no such separation. Because when you switch to rng, for example, many tests start to fail. It's almost like the test precision is tailored manually. Hence it is a bit tricky.

OK, if the rng-changes are not just stylistic but functionally relevant, then let's exclude those. For everything else, there's #18462 ;-)

I see that changes on isolve is affecting optimize tests too (scipy/optimize/tests/test_nonlin.py::TestNonlin) and they also have mixed solvers. I guess I need to do something about this indeed.

I think the last three could be tackled in preparatory PRs. If you want, I can help with the fixture one.

Alright, this has now happened with #18462 & #18463. Should be good to rebase (or merge) here. I'd still prefer

    with suppress_warnings() as sup:
        sup.filter(DeprecationWarning, ".*keyword argument 'tol' is deprecated.*")
        # TODO: ensure all solvers support rtol keyword
        x, info = solver(A, b, x0=x0, tol=tol, maxiter=1, callback=callback)

instead of

    if solver in [cg, cgs, bicg, bicgstab, gmres, qmr]:
        x, info = solver(A, b, x0=x0, rtol=tol, maxiter=1, callback=callback)
    else:
        x, info = solver(A, b, x0=x0, tol=tol, maxiter=1, callback=callback)

as noted above, for all those occurrences.

I'd prefer that too but it means going through all solvers which is not that trivial and gets a bit too much out of the scope of this PR.

I'd prefer that too but it means going through all solvers which is not that trivial and gets a bit too much out of the scope of this PR.

I must be doing quite a terrible job of explaining myself... Currently all solvers support tol=, which is why the test module can contain lines like x, info = solver(A, b, x0=x0, tol=tol, maxiter=1, callback=callback) without branches. In this PR, you're introducing an rtol= keyword for some solvers that supersedes & deprecates tol=.

What I'm saying is to leave the tests using tol=, and only filter out the warning, until we have universal support for rtol=. In other words, it reduces the scope of this PR (less test changes & the result is self-documenting), rather than increasing it.

Yes I think I have same issue. Currently we are also changing the default values of the signature. So not providing those keywords lead to different behaviors on the solvers I touched and other solvers that I didn't.

So tol=1e-5, atol=None, rtol=0. is not the same as tol=None, atol=0., rtol=1e-5. The legacy behavior is behaving way too wonky which is way I touched it.

The plan is in one of the issues here but I couldn't find it. But also in the mailing list. It is not that trivial to bunch them up with a fixture. I have to say, the idea is to get rid of Fortran solvers not unifying test file.

So tol=1e-5, atol=None, rtol=0. is not the same as tol=None, atol=0., rtol=1e-5. The legacy behavior is behaving way too wonky which is way I touched it.

OK, I understand. But does tol=1e-5, atol=None, rtol=None before this PR still mean the same as tol=1e-5, atol=None, rtol=None after this PR? That's all that's required to keep a line like x, info = solver(A, b, x0=x0, tol=tol, maxiter=1) unchanged both in terms of code & outcome.

The plan is in one of the issues here but I couldn't find it.

I think you mean this.

It is not that trivial to bunch them up with a fixture.

The fixturization is already done (but that's not a substantial change from the current state of this PR, which already had tests templated over solver).

I have to say, the idea is to get rid of Fortran solvers not unifying test file.

Sure, and it's a worthwhile goal. I'm trying to avoid unnecessary churn in the tests (and where that's not possible, to ensure the state they're left in is self-explanatory without doing git archeology).

But does tol=1e-5, atol=None, rtol=None before this PR still mean the same as tol=1e-5, atol=None, rtol=None after this PR?

No because the default of tol changed to None and fires up a different warning to not to use tol anymore. And requires another warning filter.

No because the default of tol changed to None and fires up a different warning to not to use tol anymore. And requires another warning filter.

The warning can be filtered (as my example contains), and tol=1e-5, atol=None (the default before this PR) got transformed through _get_atol to do the equivalent of the new default tol=1e-5, atol=0., namely: both before & after, tol=x (without atol) defaults to a relative accuracy of 1e-5.

Looking closer at the code to actually verify that the default behaviour (on a meta-level) did not change, I noticed also that rather than repeating

    if tol is not None:
        msg = ("'scipy.sparse.linalg.cg' keyword argument 'tol' is "
               "deprecated in favor of 'rtol' and will be removed in SciPy "
               "v.1.13.0. Until then, if set, will override 'rtol'.")
        warnings.warn(msg, category=DeprecationWarning, stacklevel=3)
        rtol = float(tol)

    if isinstance(atol, str):
        warnings.warn("scipy.sparse.linalg.cg called with `atol` set to "
                      "string, possibly with value 'legacy'. This behavior "
                      "is deprecated and atol parameter only excepts floats."
                      " In SciPy 1.13, this will result with an error.",
                      category=DeprecationWarning, stacklevel=3)

    atol = max(float(atol), rtol * float(np.linalg.norm(b)))

in each function, it'd be better to keep _get_atol and move all the logic there, rather than copying this chunk into all solvers.

I think we are again getting into the preference land. Repeated or not they are going to be removed in two versions. So DRY principle is not applicable here. Explicit removal is much better in these cases then implicit because then you know what is touched and what isn't.

This is already a very large PR and I really cannot keep track of which is yours which is mine to resolve the conflicts and after rebase now all tests are failing again so I don't see how this is helping.

Like I said, if we are going to filter new warnings that's pretty much if then else block type of intervention so if you don't mind I'm going to finish this PR with what I have then we can retouch it if there is any need.

Repeated or not they are going to be removed in two versions. So DRY principle is not applicable here.

Any later changes to that logic, or even the warning itself, must now be duplicated X times (who knows what happen in the next year that causes this to be touched) - how is that helpful? It's IMO a trivial request - keep the function that was already there for that purpose.

I think we are again getting into the preference land.

What's a review if not a collection of preferences (that are held at varying degrees of intensity)? But I see that we are not progressing, so I'll leave the review of this to someone else.

I am not really following anymore. We do things in separate PRs for a lot less in many other Pull Requests. It was PEP8 fine let's do it. It was the tests fine let's do it. Now it is the code with very very delicate state and I say maybe we leave afterwards, and I'm getting this comment? I mean come on.

Let me close this and handle the rebase on the fresh copy as it is less work.

Now it is the code with very very delicate state and I say maybe we leave afterwards, and I'm getting this comment? I mean come on.

No offense intended. From my POV: I asked for things that are IMO reasonable and relevant, you tell me repeatedly that they're not possible or not useful. Rather than block your progress with opinions that you either don't share or don't want to take into account, I'm taking myself out of the picture.

Fine, my bad then. I don't care about being right. I am already running out of patience with this code so let's be done with it in the next PR.