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

Initial infrastructure for Krylov-based linear solvers #56

Merged
merged 14 commits into from
Sep 14, 2020
Merged

Conversation

mtanneau
Copy link
Member

@mtanneau mtanneau commented Aug 26, 2020

cc @amontoison

This PR introduces some infrastructure for Krylov-based linear solvers.
There are no Krylov-specific algorithmic modifications to the IPM at this point.

TODO:

  • Identify relevant data structures and interfaces for the task
  • Decide on how to best implement preconditioners [left for future PR]
  • Add unit tests
  • Update docs

Currently, a Krylov solver can be selected as follows:

model.params.KKTOptions = Tulip.KKT.SolverOptions(
    Tulip.KKT.KrylovPDSolver,       # Solve the normal equations system with a Krylov method
    method=Tulip.KKT.Krylov.minres  # Select the Krylov method
)

the syntax also allows for passing a callback:

model.params.KKTOptions = Tulip.KKT.SolverOptions(
    Tulip.KKT.KrylovPDSolver,       # Solve the normal equations system with a Krylov method
    method=(A, b) -> Tulip.KKT.Krylov.cg(A, b)  # "custom" Krylov method
)

@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2020

Codecov Report

Merging #56 into master will decrease coverage by 0.04%.
The diff coverage is 87.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
- Coverage   89.72%   89.67%   -0.05%     
==========================================
  Files          30       31       +1     
  Lines        2073     2122      +49     
==========================================
+ Hits         1860     1903      +43     
- Misses        213      219       +6     
Impacted Files Coverage Δ
src/KKTSolver/KKTSolver.jl 77.77% <ø> (ø)
src/KKTSolver/krylov.jl 87.75% <87.75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68a3b96...2003908. Read the comment docs.

src/KKTSolver/krylov.jl Outdated Show resolved Hide resolved
src/KKTSolver/krylov.jl Outdated Show resolved Hide resolved
Comment on lines +104 to +105
v1 = zeros(Tv, n)
v2 = zeros(Tv, m)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should move v1 and v2 in KrylovPDSolver structure and allocate them only once.

D = Diagonal(d)

# Set-up right-hand side
ξ_ = ξp .+ kkt.A * (D * ξd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use v1 and v2 here to avoid 2 new allocations at each solve! pass:

mul!(v1, D, ξd)
mul!(v2, kkt.A, v1)
v2 .+= ξp

dy .= _dy

# Recover dx
dx .= D * (kkt.A' * dy - ξd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could reuse v1 and v2 :

mul!(v1, kkt.A', dy)
v1 .-= ξd
mul!(v2, D, v1)

Comment on lines 96 to 97
d = one(Float64) ./ (kkt.θ .+ kkt.regP)
D = Diagonal(d)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to form d and D explicitly. If you want to keep it because it's more user-friendly, I recommend to preallocate a vector d inside the KrylovPDSolver structure and reuse the memory.

src/KKTSolver/krylov.jl Outdated Show resolved Hide resolved
Comment on lines 216 to 217
d = one(Float64) ./ (kkt.θ .+ kkt.regP)
D = Diagonal(d)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v3 = zeros(Tv, n)
opD = LinearOperator(Tv, n, n, true, true, v -> v3 .= v ./ (kkt.θ .+ kkt.regP))

src/KKTSolver/krylov.jl Outdated Show resolved Hide resolved
@mtanneau
Copy link
Member Author

mtanneau commented Sep 4, 2020

I don't want to optimize the code too much for now.
I'll update the docs.

@mtanneau
Copy link
Member Author

I'm happy with the current state.

@amontoison I will introduce Jacobi preconditioners in a forthcoming PR.

@mtanneau mtanneau marked this pull request as ready for review September 12, 2020 02:48
@amontoison
Copy link
Contributor

Let's merge thIs PR. 👍

src/KKTSolver/krylov.jl Outdated Show resolved Hide resolved
@mtanneau
Copy link
Member Author

mtanneau commented Sep 13, 2020

One last issue: unified syntax for the resolution of SQD systems.

For functions like minres or minres_qlp, things look like

K = [-(Θ⁻¹ + Rp) Aᵀ; A Rd]  # obviously this would be a linear operator
Δ, stats = minres(K, ξ)  # solve 2x2 system

# Recover search directions
dx .= Δ[1:n]
dy .= Δ[n+1:m]

while for trimr we have

_dx, _dy, stats = trimr(A, ξp, ξd)  # solve 2x2 system

# Recover search directions
dx .= _dx
dy .= _dy

We could have an internal flag for KrylovSQDSolvers that indicates whether the method exploits the 2x2 block structure or not, but I would prefer to use dispatch instead.

So, I suggest to have a KrylovSIDSolver type for methods that solve the Symmetric InDefinite augmented system without exploiting the SQD structure explicitly.

@amontoison
Copy link
Contributor

I prefer the second solution with a KrylovSIDSolver type.

src/KKTSolver/krylov.jl Outdated Show resolved Hide resolved
@mtanneau mtanneau merged commit 524b39b into master Sep 14, 2020
@mtanneau mtanneau deleted the Krylov branch September 14, 2020 17:22
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