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

Fix preconditioning in cg when optional radius argument is positive #868

Merged
merged 5 commits into from
Aug 5, 2024

Conversation

mpf
Copy link
Contributor

@mpf mpf commented May 30, 2024

As highlighted by @amontoison in this issue, the step-to-the-boundary computation in to_boundary needs to be updated so that it solves the equation

‖x + σd‖_M = radius

where ‖v‖_M^2 = v'Mv. This pull request implements the required changes to to_boundary.

Note:

  • cg is the only calling routine that's been correspondingly modified. Presumably all other solvers will also need to be suitably modified.
  • the modification to to_boundary implements two products with the preconditioner in order to compute Md and Mx. It should be possible to avoid these products by instead accummulating the products in the calling cg routine.

See also this JSOSolvers.jl issue.

@amontoison
Copy link
Member

amontoison commented May 30, 2024

Thanks @mpf!
Can you update these tests: https://github.com/JuliaSmoothOptimizers/Krylov.jl/blob/main/test/test_aux.jl#L104-L116 ?
It should fix the errors.

Copy link
Contributor

github-actions bot commented May 30, 2024

Package name latest stable
CaNNOLeS.jl
DCISolver.jl
FletcherPenaltySolver.jl
JSOSolvers.jl
LLSModels.jl
LinearSolve.jl
Percival.jl
RipQP.jl

Note the new 4th argument z, which is a vector that provides workspace for the mul! (or ldiv!) operations on the preconditioner. This isn't needed when M=I`.

Because z is modified in place, probably the function should be renamed to to_boundary! (add exclamation).
@amontoison amontoison marked this pull request as ready for review May 31, 2024 01:31
@amontoison
Copy link
Member

amontoison commented Aug 3, 2024

@mpf Do you confirm that if we use a trust region in CG (and other Krylov solvers like CR, CRLS, CGLS, LSQR, LSMR), we need both ldiv! and mul! methods for the preconditioner?

I don't remember why the norm of the trust region is defined by an elliptic norm induced by the inverse of the preconditioner.

I need to update the docstings to specify that after your PR.

@dpo
Copy link
Member

dpo commented Aug 4, 2024

I don't remember why the norm of the trust region is defined by an elliptic norm induced by the inverse of the preconditioner.

@amontoison say the trust-region constraint is s’Ms ≤ Δ². Then change variable: t = M^{½} s.

@amontoison
Copy link
Member

@dpo Thanks! Can you confirm that we must provide both functions ldiv! and mul! if we use a trust-region?
We only use M \ v or M^{-1} * v for the Lanczos process and not M * v.

@dpo
Copy link
Member

dpo commented Aug 4, 2024

The role of to_boundary() is to solve the quadratic equation $\Vert x + \alpha d \Vert_M^2 = \Delta^2$ for $\alpha$, i.e.,
$$\alpha^2 \Vert d \Vert_M^2 + 2 \alpha x^T M d + (\Vert x \Vert_M^2 - \Delta^2) = 0.$$

Currently, the user supplies $\Vert x \Vert_2$ and $\Vert d \Vert_2$, and we compute $x^T d$. But it would be just as easy to ask the user to supply $\Vert x \Vert_M$, $\Vert d \Vert_M$ and $x^T M d$, so that we would not require mul!() with $M$.

@mpf
Copy link
Contributor Author

mpf commented Aug 5, 2024

Currently, the user supplies ‖x‖2 and ‖d‖2, and we compute xTd. But it would be just as easy to ask the user to supply ‖x‖M, ‖d‖M and xTMd, so that we would not require mul!() with M.

That would indeed simplify to_boundary. But in that case, the preconditioned Krylov solver that calls to_boundary would need to compute these quantities, and thus require both mul!() and ldiv!().

Gould, Lucidi, Roma, and Toint [SIAM Opt, 1999, p.514] describe an approach that tracks $\Vert x+\alpha d\Vert_M$ iteratively. Perhaps that approach would avoid both mul! and ldiv! for some Krylov solvers?

@amontoison
Copy link
Member

@mpf Can you merge this PR ou your fork?
mpf#1

Update the arguments of to_boundary
@amontoison amontoison merged commit 3e915b7 into JuliaSmoothOptimizers:main Aug 5, 2024
29 of 34 checks passed
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