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.

Sign up for GitHub

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

Jump to bottom

Add solve with Krylov methods #35

Merged
merged 4 commits into from
Mar 21, 2022
Merged

Add solve with Krylov methods #35

merged 4 commits into from
Mar 21, 2022

Conversation

tmigot
Copy link
Member

@tmigot tmigot commented Feb 15, 2022

Following the discussion here JuliaSmoothOptimizers/Krylov.jl#540

This could be documented in a separate tutorial.

@codecov
Copy link

codecov bot commented Feb 15, 2022 •
edited
Loading

Codecov Report

Merging #35 (338ee05) into main (ffa2812) will increase coverage by 0.58%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
+ Coverage   94.91%   95.50%   +0.58%     
==========================================
  Files           2        4       +2     
  Lines         177      200      +23     
==========================================
+ Hits          168      191      +23     
  Misses          9        9
Impacted Files Coverage Δ
src/LLSModels.jl 100.00% <ø> (ø)
src/solve_krylov.jl 100.00% <100.00%> (ø)

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 ffa2812...338ee05. Read the comment docs.

amontoison
amontoison reviewed Feb 15, 2022
View reviewed changes
src/solve_krylov.jl Outdated Show resolved Hide resolved
dpo
dpo requested changes Feb 15, 2022
View reviewed changes
Project.toml Outdated Show resolved Hide resolved
src/solve_krylov.jl Outdated Show resolved Hide resolved
src/solve_krylov.jl Outdated Show resolved Hide resolved
@amontoison
Copy link
Member

amontoison commented Feb 15, 2022 •
edited
Loading

@tmigot you could also add specialized methods for the Krylov solvers:

@inline Krylov.$(KS)(lls::LLSModel) = Krylov.$(KS)(lls.A, lls.b)

The user will be able to use all the features of Krylov.jl without using lls.A and lls.b.

lls = LLSModel(A, b)
(x, stats) = cgls(lls)
solver = CglsSolver(lls)
cgls!(solver, lls)

@amontoison amontoison mentioned this pull request Feb 15, 2022
Closed
amontoison
amontoison reviewed Feb 18, 2022
View reviewed changes
src/solve_krylov.jl Outdated Show resolved Hide resolved
@tmigot tmigot requested a review from dpo February 18, 2022 15:15
dpo
dpo reviewed Feb 19, 2022
View reviewed changes
Copy link
Member

@dpo dpo left a comment

Choose a reason for hiding this comment

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

Looks good, but it seems we're restricting the user to a handful of solvers that are made specifically for least squares. I see no reason to prevent users from calling any other method (that might or might not work). For example, MINRES and GMRES are also legal choices. If the least-squares problem is consistent, CRAIG, SYMMLQ, etc., are also legal choices. Why not let the user call any solver?

src/solve_krylov.jl Outdated Show resolved Hide resolved
dpo
dpo requested changes Feb 19, 2022
View reviewed changes
Copy link
Member

@dpo dpo left a comment

Choose a reason for hiding this comment

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

Also, please add a little documentation about this feature.

@tmigot tmigot mentioned this pull request Feb 28, 2022
Merged
@dpo
Copy link
Member

dpo commented Mar 1, 2022

No merge commits please. Use rebase instead.

@tmigot tmigot marked this pull request as draft March 1, 2022 20:48
@tmigot
add solve with Krylov
01472ef 
add docstring

return error for constrained problems

fix comments

add tailored solver structure

use `Krylov.KRYLOV_SOLVERS`

type `Dict`

Co-authored-by: Alexis <35051714+amontoison@users.noreply.github.com>

Update src/solve_krylov.jl

Co-authored-by: Dominique <dominique.orban@gmail.com>

fix tests

fix
@tmigot tmigot marked this pull request as ready for review March 2, 2022 05:02
dpo
dpo reviewed Mar 2, 2022
View reviewed changes
src/solve_krylov.jl Outdated Show resolved Hide resolved
@tmigot tmigot merged commit f043eb5 into main Mar 21, 2022
@tmigot tmigot deleted the add-solve branch March 21, 2022 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amontoison amontoison amontoison approved these changes

@dpo dpo dpo requested changes

@geoffroyleconte geoffroyleconte geoffroyleconte approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tmigot @amontoison @dpo @geoffroyleconte