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

L2 Penalty Algorithm #145

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

MaxenceGollier
Copy link
Contributor

This is my implementation of the $\ell_2$ exact penalty algorithm.
Details are provided in the documentation.

Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@@ -20,5 +20,8 @@ include("TRDH_alg.jl")
include("R2_alg.jl")
include("LM_alg.jl")
include("LMTR_alg.jl")
include("R2DH.jl")
include("R2N.jl")
Copy link
Member

Choose a reason for hiding this comment

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

Remove R2N and R2DH from here. Also, if you want to update LM, please do it in a separate PR.

src/L2Penalty_alg.jl Outdated Show resolved Hide resolved
ktol = max(ktol,atol) # Keep ϵ₀ ≥ ϵ
tol_init = ktol # store value of ϵ₀

done = false
Copy link
Member

Choose a reason for hiding this comment

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

Should compute the least-squares multipliers here. If by chance, the solver were initialized with a stationary point, we should exit before the main loop.

n_iter_since_decrease = 0

while !done
model = RegularizedNLPModel(nlp, sub_ψ)
Copy link
Member

Choose a reason for hiding this comment

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

This probably allocates a little. We should think of a way to create it once and update sub_ψ.

src/L2Penalty_alg.jl Outdated Show resolved Hide resolved
end
set_solution!(stats,x1[1:n])

end
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this function be in ShiftedProximalOperators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really know.

I think it should be considered as a JSO solver since this is what R2N will expect as a subsolver.
Therefore, it should be written here instead of ShiftedProximalOperators but I am open to rewrite it elsewhere if necessary.

H1 = [-Q reg_nlp.h.A']
H2 = [reg_nlp.h.A αₖ*opEye(m,m)]
H = [H1;H2]
x1,_ = minres_qlp(H,u1)
Copy link
Member

Choose a reason for hiding this comment

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

Here, we should explore the potential of other methods, such as TriMR.

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.

@MaxenceGollier Please revise this PR.

@MaxenceGollier
Copy link
Contributor Author

@dpo thank you for the review, can we merge #153 fast ? I can then add a separate PR for my version of R2N and we can then merge this.

@dpo
Copy link
Member

dpo commented Oct 31, 2024

@dpo thank you for the review, can we merge #153 fast ? I can then add a separate PR for my version of R2N and we can then merge this.

First, @MohamedLaghdafHABIBOULLAH should revise #153.

@MohamedLaghdafHABIBOULLAH
Copy link
Contributor

I should also add some unit test for R2N and R2DH.

@@ -4,6 +4,8 @@ author = ["Robert Baraldi <[email protected]> and Dominique Orban <dominique.orban
version = "0.1.0"

[deps]
DelimitedFiles = "8bb1440f-4735-579b-a4ab-409b98df4dab"
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? It shouldn't be required by the solver.

@@ -104,13 +104,15 @@ function LM(
k = 0
Fobj_hist = zeros(maxIter)
Hobj_hist = zeros(maxIter)
R = eltype(xk)
sqrt_ξ1_νInv = one(R)
Copy link
Member

Choose a reason for hiding this comment

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

Please do this in a separate PR.

@@ -0,0 +1,297 @@
export R2DH
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove R2N and R2DH from this PR.

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