Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Line Search with Negative Curvature Detection for MINRES Based on Liu et al. (2022) #969

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

farhadrclass
Copy link
Contributor

Description:
This pull request introduces a new line search routine to the MINRES algorithm, incorporating negative curvature detection as outlined in Algorithm 1 of:

Liu, Yang & Roosta, Fred. (2022). A Newton-MR algorithm with complexity guarantees for nonconvex smooth unconstrained optimization. [10.48550/arXiv.2208.07095](https://doi.org/10.48550/arXiv.2208.07095)

Key Changes:

  • Negative Curvature Detection:

    • The implementation now detects negative curvature based on Algorithm 1 from the paper.
    • When negative curvature is detected:
      • If the linesearch flag is enabled and it occurs on the first iteration, the algorithm returns b.
      • Otherwise, it returns the last computed r1.
  • Linesearch Flag:

    • Introduced a new flag to enable or disable the line search mechanism, allowing users to control the enhanced step size adjustments.
  • Testing & Documentation:

    • Updated tests and documentation to cover the new negative curvature detection and line search behavior.

@farhadrclass farhadrclass self-assigned this Feb 24, 2025
@farhadrclass farhadrclass requested a review from dpo February 24, 2025 17:20
@farhadrclass
Copy link
Contributor Author

@dpo
I think we need to test this case

if β₁ == 0
      stats.niter = 0
      stats.solved, stats.inconsistent = true, false
      stats.timer = start_time |> ktimer
      stats.status = "b is a zero-curvature directions"
      history && push!(rNorms, β₁)
      history && push!(ArNorms, zero(T))
      history && push!(Aconds, zero(T))
      warm_start && kaxpy!(n, one(FC), Δx, x)
      solver.warm_start = false
      linesearch && kcopy!(n, x, b) # x ← b
      return solver
    end

But all the systems I test or create does not have \beta_1 == 0, any suggestions on a system that would force this case?

Copy link

codecov bot commented Feb 24, 2025

Codecov Report

Attention: Patch coverage is 97.80220% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.58%. Comparing base (9536ef7) to head (d30705c).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
src/block_krylov_solvers.jl 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #969      +/-   ##
==========================================
- Coverage   94.68%   93.58%   -1.10%     
==========================================
  Files          45       47       +2     
  Lines        8027     9235    +1208     
==========================================
+ Hits         7600     8643    +1043     
- Misses        427      592     +165     

☔ 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
Copy link
Member

dpo commented Feb 25, 2025

$$\beta_1 = 0$$ means $$b = 0$$.

@farhadrclass
Copy link
Contributor Author

then this line is not that important
linesearch && kcopy!(n, x, b) # x ← b ?

@dpo
Copy link
Member

dpo commented Feb 26, 2025

then this line is not that important linesearch && kcopy!(n, x, b) # x ← b ?

If b = 0, the optimizer that calls MINRES should already have stopped. So indeed, that line is not necessary because x is already zero at that point.

@farhadrclass
Copy link
Contributor Author

farhadrclass commented Mar 4, 2025

@dpo

I've updated the code and status to include a flag for non-positive curvature and line search.

  • If line search is enabled and we detect a non-positive or zero curvature, we compute rk and return it.
  • I'm not entirely sure about the scaling—could you review my logic?
  • I also need to rethink some tests and update the test cases. Additionally, I need to add tests for the new statistics.
  • allocate an extra vector to recur the residual vector when linesearch == true.
  • For now, I suggest introducing a conjugate stats struct, as we'll reuse it for both the CR , MINRES, and CG solvers soon. There we have 2 new flags
  • nonpositive curv
  • linesearch
    User then can check them and if both are true, take advantage of the linesearch

Additionally, in MINRES, we currently use r1 and r2. I propose using rk as the residual when line search is enabled.

Let me know your thoughts!

To-Do List:

  1. Add tests for statistics.

  2. Verify that the returned solution is indeed a direction of non-positive curvature.

  3. From what I recall in the last PR, @amontoison mentioned he would handle ensuring that line search and warm start are not simultaneously enabled.

  4. I suggest (based on what @dpo suggested) I moved the rk to stats and then return x in all cases and user can decide which one to use if needed, however the allocation would be needed if we move if to stats and any suggestion? (I will send the new updated code shortly)

  5. I think we should keep the solver.rk, when we use the linesearch and encounter negative curvature, we return the last residual xk but user has access to rk from solver.rk

@dpo
Copy link
Member

dpo commented Mar 4, 2025

From what I recall in the last PR, @amontoison mentioned he would handle ensuring that line search and warm start are not simultaneously enabled.

@farhadrclass Please do it. It’s not difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants