Use combined tolerance for solved check

[farhadrclass]

Replace the previous rNorm ≤ rtol check with rNorm ≤ ε (where ε = atol + rtol * β₁) when setting solved, solved_mach, and solved_lim. This makes the convergence/zero-residual detection consistent with the computed combined tolerance (absolute+relative) and prevents incorrect termination when atol or the initial scale β₁ are nonzero.

[farhadrclass]

@dpo this is the issue,
It was a small fix,

[Copilot]

Pull request overview

Updates MINRES’ initial “solved” detection to use the same combined absolute+relative tolerance used elsewhere (ε = atol + rtol * β₁), avoiding incorrect termination behavior when atol or the initial residual scale is nonzero.

Changes:

• Replace the initial rNorm ≤ rtol solved check with rNorm ≤ ε (combined tolerance).
• Align initialization of solved, solved_mach, and solved_lim with the already-computed ε.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dpo]

That seems right. It's also the test used in MINRES-QLP: https://github.com/JuliaSmoothOptimizers/Krylov.jl/blob/main/src/minres_qlp.jl#L480

[farhadrclass]

@dpo I agree and also the same test in CR and CG,

Also I don't agree with co-pilot review so I ignored it

If it is good for you, should we merge it ?

[amontoison]

We use 6 variables for the same stopping condition, maybe we should just removed a few of them.
What you want is already at line 273:
zero_resid = zero_resid_mach = zero_resid_lim = (rNorm ≤ ε).

I don't understand why we have so many variables.

[farhadrclass]

@dpo and @amontoison
I agree but the goal of this PR is just to fix the current issue that is blocking JSO solver from using MINREs

[dpo]

Is it a typo? Should it be ArNorm in the line above?

[amontoison]

It will change the returned status at the end. With your modifications, we have two times the same stopping conditions but the related variables are used for different status.

[dpo]

At the end of the main loop, solved is set based on $$\Vert A r \Vert$$, not $$\Vert r \Vert$$: https://github.com/JuliaSmoothOptimizers/Krylov.jl/pull/1079/changes#diff-65c1dbcc771522673d6f0c4e47c90e950b2bcb251ceda42f6de20901c3055021R449

So I believe the line that @farhadrclass modified should instead be

solved = solved_mach = solved_lim = (ArNorm ≤ ε)

There could also be an additional test

zero_resid = zero_resid_mach = zero_resid_linm = (rNorm ≤ ε)

Those are stopping conditions of different natures.

[dpo]

The condition with test2 is about ArNorm.

[farhadrclass]

At the end of the main loop, solved is set based on ‖ A r ‖ , not ‖ r ‖ : https://github.com/JuliaSmoothOptimizers/Krylov.jl/pull/1079/changes#diff-65c1dbcc771522673d6f0c4e47c90e950b2bcb251ceda42f6de20901c3055021R449

So I believe the line that @farhadrclass modified should instead be

solved = solved_mach = solved_lim = (ArNorm ≤ ε)

There could also be an additional test

zero_resid = zero_resid_mach = zero_resid_linm = (rNorm ≤ ε)

Those are stopping conditions of different natures.

yes I agree but then why in minres_qlp we have
solved = zero_resid = zero_resid_lim = rNorm ≤ ε

[farhadrclass]

should we have solved = zero_resid?

[farhadrclass]

Also ArNorm is zero before the loop at line 254

[farhadrclass]

@dpo @amontoison I agree with about the difference between the exact solution (rNorm) and the least-squares solution (ArNorm).

@dpo, However, to handle the pre-loop initialization, I can't check (ArNorm ≤ ε) because ArNorm is initialized to 0 at line 254. Evaluating it before the loop would cause the solver to instantly terminate on every problem!

Instead of hardcoding the flags to false or calculating ‖Ab‖ before the loop, I think the cleanest fix is to just use (rNorm ≤ ε) for the pre-loop check. Mathematically, if the initial exact residual is below tolerance, it is also a valid least-squares solution ($r=0 \implies A^T r = 0$), no?

@dpo what do you think?
I also think if we want to rethink the variables, that would be a bigger PR and should include more solvers not just MINRES and that would my attention away from JSOSolvers now

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.69%. Comparing base (9536ef7) to head (20273bc).
⚠️ Report is 123 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1079      +/-   ##
==========================================
+ Coverage   94.68%   97.69%   +3.01%     
==========================================
  Files          45       50       +5     
  Lines        8027    10011    +1984     
==========================================
+ Hits         7600     9780    +2180     
+ Misses        427      231     -196     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dpo]

Why is documentation failing?

[amontoison]

Why is documentation failing?

It is related to a tutorial with FFTW.jl for linear operators.
It is not related to this PR.

I will fix it when I will be allowed to commit again.

[farhadrclass]

@dpo I think @amontoison is right

I think the issue with LaplacianOperator being negative defined no?

[dpo]

Thank you!