Add R2N, R2DH #153

MohamedLaghdafHABIBOULLAH · 2024-09-12T16:00:11Z

Add still allocating versions of R2N and R2DH.
Update LM to give the possibility to use R2DH as subsolver

dpo

Please update LM in a separate PR. It's unrelated to R2N.

dpo · 2024-09-15T14:30:17Z

src/LM_alg.jl

@@ -244,6 +248,7 @@ function LM(
      jtprod_residual!(nls, xk, Fk, ∇fk)

      σmax = opnorm(Jk)
+


Please don't add redundant blank lines.

dpo · 2024-09-15T14:32:43Z

src/LM_alg.jl

@@ -252,6 +257,7 @@ function LM(
      σk = σk * γ
    end
    νInv = (1 + θ) * (σmax^2 + σk)  # ‖J'J + σₖ I‖ = ‖J‖² + σₖ
+


Not here either.

MohamedLaghdafHABIBOULLAH · 2024-09-17T05:45:31Z

@dpo here I add a test in line 194 to discuss the case if $$mk(s)$$ is big enough

MohamedLaghdafHABIBOULLAH · 2024-09-17T07:15:18Z

@dpo il est à noter que je me suis permis de changer R2_alg.jl afin de pouvoir récupérer le temps pour chaque iter et tracer la courbe objective vs temps

dpo · 2024-09-17T12:01:00Z

@dpo il est à noter que je me suis permis de changer R2_alg.jl afin de pouvoir récupérer le temps pour chaque iter et tracer la courbe objective vs temps

dpo · 2024-09-16T12:50:06Z

src/R2DH.jl

+
+* `x0::AbstractVector`: an initial guess (in the first calling form: default = `nlp.meta.x0`)
+* `selected::AbstractVector{<:Integer}`: (default `1:length(x0)`).
+* `Bk`: initial diagonal Hessian approximation (default: `(one(R) / options.ν) * I`).


Cette interface ne prend pas ce kwarg.

Please fix.

dpo · 2024-09-16T12:52:22Z

src/R2DH.jl

+  D.d .= summation ? D.d .+ σk : D.d  .* σk
+  DNorm = norm(D.d, Inf)
+
+


Remove all duplicate blank lines.

dpo · 2024-09-16T12:52:53Z

src/R2DH.jl

+    Hobj_hist[k] = hk
+    Mmonotone > 0 && (FHobj_hist[mod(k-1, Mmonotone) + 1] = fk + hk)
+
+    D.d .= max.(D.d, eps(R))


Why do we need to do this?

dpo · 2024-09-17T12:01:53Z

src/R2_alg.jl

@@ -21,6 +21,7 @@ mutable struct R2Solver{
  Fobj_hist::Vector{R}
  Hobj_hist::Vector{R}
  Complex_hist::Vector{Int}
+  Time_hist::Vector{R}


On veut se débarrasser de Fobj_hist, etc. Les callbacks sont là pour ça. Il vaut mieux utiliser le callback.

Ok je vais ouvrir une issue pour ça

Don’t change R2 in this PR.

dpo · 2024-10-15T18:09:30Z

src/R2_alg.jl

@@ -21,6 +21,7 @@ mutable struct R2Solver{
  Fobj_hist::Vector{R}
  Hobj_hist::Vector{R}
  Complex_hist::Vector{Int}
+  Time_hist::Vector{R}


Don’t change R2 in this PR.

dpo · 2024-10-15T18:10:29Z

src/input_struct.jl

@@ -9,6 +9,7 @@ mutable struct ROSolverOptions{R}
  maxIter::Int  # maximum amount of inner iterations
  maxTime::Float64 #maximum time allotted to the algorithm in s
  σmin::R # minimum σk allowed for LM/R2 method
+  σk::R # initial σk


LM did not need this. R2N and LM should be consistent.

In contrast to the old code, we need an initialization $$\sigma_k$$, similar to $$\Delta_k$$ as well as a different $$\sigma_{min}$$ for which there is not really a relationship between them.

src/R2N.jl

src/R2DH.jl

dpo · 2024-10-28T15:08:51Z

src/R2DH.jl

+    min  φ(s; xₖ) + ψ(s; xₖ)
+
+where φ(s ; xₖ) = f(xₖ) + ∇f(xₖ)ᵀs + ½ sᵀ (σₖ+Dₖ) s is a quadratic approximation of f about xₖ,
+ψ(s; xₖ) = h(xₖ + s), ‖⋅‖ is a user-defined norm, Dₖ is a diagonal Hessian approximation


There’s no norm here.

dpo · 2024-10-28T15:09:01Z

src/R2DH.jl

+
+* `x0::AbstractVector`: an initial guess (in the first calling form: default = `nlp.meta.x0`)
+* `selected::AbstractVector{<:Integer}`: (default `1:length(x0)`).
+* `Bk`: initial diagonal Hessian approximation (default: `(one(R) / options.ν) * I`).


Please fix.

dpo · 2024-10-28T15:10:32Z

src/R2DH.jl

+  ∇f!(∇fk, xk)
+  ∇fk⁻ = copy(∇fk) 
+  spectral_test = isa(D, SpectralGradient)
+ # D.d .= D.d .+ σk


Remove dead code.

dpo · 2024-10-28T15:11:18Z

src/R2DH.jl

+    end
+    mks = mk(s)
+
+    if mks < -1e5


What is this for???

This is triggered when $m_{ks}$ is very small (approaching $-\infty$). In such cases, we do not need to compute $f(x_{kn})$, and we simply assume that the iteration is unsuccessful.

I don’t understand why we would need that. Firstly, -1e5 is completely arbitrary. And secondly, why would the iteration be unsuccessful if the model decrease is large?

Because of this (page 7 of R2N)

So $$\rho_k = 0$$
Hence the iteration is automatically uncessuful

However, I agree that -1e5 might be somewhat arbitrary. Should we set it to -Inf instead?
We should also ensure that the ShiftedProximalOperators handle this appropriately when encountering $d[i] < 0$.

My concern is that Julia might struggle with calculus involving -Inf and interpret it as NaN. However, this does not seem to be the case, except in scenarios like $\text{Inf} - \text{Inf}$ or $\frac{\text{Inf}}{\text{Inf}}$, which yield NaN. In the paper, the latter, which may be $\rho$, should be equal to 0 by convention, but Julia treats it as NaN.

It should be -Inf, and ShiftedProximalOperators should return -Inf.

MohamedLaghdafHABIBOULLAH · 2024-10-31T19:16:10Z

Thanks @dpo for the comments. I tried to incorporate and answer all your comments.
Now I need to add some unitests

src/R2DH.jl

src/R2N.jl

src/R2DH.jl

dpo · 2024-11-12T15:38:08Z

src/R2DH.jl

+    end
+    mks = mk(s)
+
+    if mks < -1e5


It should be -Inf, and ShiftedProximalOperators should return -Inf.

dpo · 2024-11-12T15:38:57Z

src/R2N.jl

+
+    min f(x) + h(x)
+
+where f: ℝⁿ → ℝ is C¹ and h: ℝⁿ → ℝ is lower semi-continuous and proper.


and prox-bounded.

src/R2N.jl

MaxenceGollier · 2024-12-19T10:44:01Z

This PR has been open for a while now, can we merge it ? @MohamedLaghdafHABIBOULLAH @dpo
I will open a PR thereafter that will remove the allocations.

dpo · 2024-12-23T19:25:00Z

I would much prefer to review one PR instead of two. Could you please put your work together into a single PR? Maybe just close this one and open a new one?!

test/runtests.jl

dpo · 2025-01-09T17:27:08Z

@MohamedLaghdafHABIBOULLAH Please rebase this branch against main.

dpo

@MohamedLaghdafHABIBOULLAH Let’s please finish this fast.

src/R2N.jl

src/R2DH.jl

dpo · 2025-01-09T18:12:41Z

src/R2DH.jl

+      ν = 1 / ((DNorm + σk) * (1 + θ))
+      @. mν∇fk = -ν * ∇fk
+      continue
+    end


What is this bit for? If it is to increase σk until the prox returns a finite value, there should be a while. Otherwise, please remove it. It doesn’t appear in any other solver.

This is occurs when mks = -Inf: so we update DNorm and ν for spectral case.
Because there is a distinction between iprox in spectral (which is a prox) and other regularizers (e.g., PSB, Andrei), as rank and nuclear norm regularizers do not have an iprox.

dpo · 2025-01-09T18:13:05Z

src/R2DH.jl

+    if mks == -Inf
+      σk = σk * γ
+      Dkσk .= D.d .+ σk
+      DNorm = norm(D.d, Inf)


Suggested change

DNorm = norm(D.d, Inf)

Same reason #153 (comment)

D has not changed here. I don’t see why you would recompute its norm.

MaxenceGollier · 2025-01-10T15:31:09Z

@dpo, are we planning to merge this first in the end ?

I would much prefer to review one PR instead of two. Could you please put your work together into a single PR? Maybe just close this one and open a new one?!

Some of your comments are already taken care of in my version

What is this bit for? If it is to increase σk until the prox returns a finite value, there should be a while. Otherwise, please remove it. It doesn’t appear in any other solver.

Co-authored-by: Dominique <[email protected]>

precise when to debug Co-authored-by: Dominique <[email protected]>

correct $\Delta_{mod}$ Co-authored-by: Dominique <[email protected]>

Add some unit tests for R2DH/R2N/R2N_R2DH

Co-authored-by: Dominique <[email protected]>

I will add (-Inf) condition in ShiftedProximalOperators as well

Co-authored-by: Dominique <[email protected]>

Co-authored-by: Maxence Gollier <[email protected]>

Add a documentation for second calling form of R2DH

MohamedLaghdafHABIBOULLAH · 2025-01-10T20:54:09Z

Thanks very much @dpo and @MaxenceGollier for the comments and commit suggestions. Incorporated all suggestions in this PR.

dpo · 2025-01-10T21:56:14Z

@MaxenceGollier What are the errors in R2DH?

dpo · 2025-01-13T21:49:22Z

src/R2N.jl

+    # take first proximal gradient step s1 and see if current xk is nearly stationary
+    # s1 minimizes φ1(s) + ‖s‖² / 2 / ν + ψ(s) ⟺ s1 ∈ prox{νψ}(-ν∇φ1(0)).
+
+    subsolver_options.ν = 1 / νInv


Here, R2N should initialize subsolver_options.σk if the subsolver is R2DH.

R2DH should receive an initial σk and compute ν based on that (currently, it does it the other way around).

MohamedLaghdafHABIBOULLAH · 2025-01-13T22:18:28Z

src/R2N.jl

+
+    subsolver_options.ϵa = k == 1 ? 1.0e-3 : min(sqrt_ξ1_νInv ^ (1.5) , sqrt_ξ1_νInv * 1e-3)
+    verbose > 0 && @debug "setting inner stopping tolerance to" subsolver_options.optTol
+    subsolver_args = subsolver == R2DH ? (SpectralGradient(νInv, f.meta.nvar),) : ()


@dpo what do you think about this line?

If I consider $$\sigma_k$$ as input to R2DH, should I let $$\nu$$ as the initialization of the diagonal Hessian approximation for the SpectralGradient as it is done here in TR in the case where TRDH is the sub solver https://github.com/JuliaSmoothOptimizers/RegularizedOptimization.jl/blob/master/src/TR_alg.jl#L189

dpo · 2025-01-14T02:21:11Z

src/R2DH.jl

+    mk(d) = φ(d) + ψ(d)
+
+    if spectral_test
+      prox!(s, ψ, mν∇fk, ν)


This doesn’t correspond to R2DH; I think this should use ν = 1 / (DNorm + σk).

In the paper, we say that R2DH is R2N where $$B_k$$ is diagonal, so for the coherence, I thought we should stick with $$\nu$$ of R2N which is ν = θ / (DNorm + σk) where θis close to one.

MohamedLaghdafHABIBOULLAH · 2025-01-14T20:57:47Z

@dpo @MaxenceGollier, if there are no further comments, we can proceed with the merge.

dpo requested changes Sep 15, 2024

View reviewed changes

MohamedLaghdafHABIBOULLAH force-pushed the R2N-R2DH branch 3 times, most recently from 4700798 to b6cd089 Compare September 17, 2024 05:43

dpo closed this Sep 17, 2024

dpo reopened this Sep 17, 2024

MohamedLaghdafHABIBOULLAH force-pushed the R2N-R2DH branch 2 times, most recently from acaad65 to 652701d Compare September 17, 2024 19:29

MohamedLaghdafHABIBOULLAH force-pushed the R2N-R2DH branch from e43dbc0 to 1500d76 Compare September 27, 2024 21:41

MohamedLaghdafHABIBOULLAH changed the title ~~Add R2N, R2DH and update LM~~ Add R2N, R2DH Sep 27, 2024

MohamedLaghdafHABIBOULLAH mentioned this pull request Sep 27, 2024

update LM solver #156

Open

dpo requested changes Oct 3, 2024

View reviewed changes

dpo requested changes Oct 15, 2024

View reviewed changes

MohamedLaghdafHABIBOULLAH force-pushed the R2N-R2DH branch from 6cb845f to 3245e55 Compare October 21, 2024 17:18

dpo requested changes Oct 28, 2024

View reviewed changes

MaxenceGollier mentioned this pull request Oct 31, 2024

L2 Penalty Algorithm #145

Closed

dpo reviewed Nov 1, 2024

View reviewed changes

src/R2DH.jl Outdated Show resolved Hide resolved

dpo reviewed Nov 1, 2024

View reviewed changes

src/R2N.jl Outdated Show resolved Hide resolved

dpo requested changes Nov 12, 2024

View reviewed changes

MaxenceGollier reviewed Jan 7, 2025

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

MaxenceGollier mentioned this pull request Jan 7, 2025

Non allocating R2N and R2DH #166

Open

dpo requested changes Jan 9, 2025

View reviewed changes

MohamedLaghdafHABIBOULLAH and others added 12 commits January 10, 2025 14:54

Update src/R2N.jl

d8334ec

Co-authored-by: Dominique <[email protected]>

Update src/R2N.jl

5bd003b

precise when to debug Co-authored-by: Dominique <[email protected]>

Update src/R2N.jl

632cdac

correct $\Delta_{mod}$ Co-authored-by: Dominique <[email protected]>

Update documentation and remove dead code

bb790c1

Update runtests.jl

73e69ba

Add some unit tests for R2DH/R2N/R2N_R2DH

Update src/R2DH.jl

3a9ebc6

Update src/R2N.jl

fd61d30

Co-authored-by: Dominique <[email protected]>

update Mmonotone as in the paper

822507c

Update src/R2DH.jl

072a6e8

Co-authored-by: Dominique <[email protected]>

Update src/R2DH.jl

c90bbc2

Co-authored-by: Dominique <[email protected]>

Update doc

a8ac466

Update R2DH.jl

f817b6c

I will add (-Inf) condition in ShiftedProximalOperators as well

MohamedLaghdafHABIBOULLAH force-pushed the R2N-R2DH branch from 78aa861 to f817b6c Compare January 10, 2025 19:57

MohamedLaghdafHABIBOULLAH and others added 9 commits January 10, 2025 15:00

Update src/R2N.jl

0fdc522

Co-authored-by: Dominique <[email protected]>

Update src/R2N.jl

a879c49

Co-authored-by: Dominique <[email protected]>

Update src/R2N.jl

7999f38

Co-authored-by: Dominique <[email protected]>

Update src/R2N.jl

dad028a

Co-authored-by: Dominique <[email protected]>

Update src/R2DH.jl

59139f0

Co-authored-by: Dominique <[email protected]>

Update src/R2DH.jl clean documentation

c2bb8ab

Co-authored-by: Dominique <[email protected]>

Update src/R2DH.jl remove redundant DNorm

d363f9e

Co-authored-by: Dominique <[email protected]>

Update test/runtests.jl

f173f2c

Co-authored-by: Maxence Gollier <[email protected]>

Update doc R2DH.jl

5eb9351

Add a documentation for second calling form of R2DH

dpo reviewed Jan 13, 2025

View reviewed changes

MohamedLaghdafHABIBOULLAH commented Jan 13, 2025

View reviewed changes

dpo reviewed Jan 14, 2025

View reviewed changes

MohamedLaghdafHABIBOULLAH added 2 commits January 14, 2025 15:45

Remove the condition mk = -Inf

635b90a

update sigma of the subsolver

cfaabd3

		@@ -244,6 +248,7 @@ function LM(
		jtprod_residual!(nls, xk, Fk, ∇fk)

		σmax = opnorm(Jk)

		D.d .= summation ? D.d .+ σk : D.d .* σk
		DNorm = norm(D.d, Inf)


		min f(x) + h(x)

		where f: ℝⁿ → ℝ is C¹ and h: ℝⁿ → ℝ is lower semi-continuous and proper.

Add R2N, R2DH #153

Are you sure you want to change the base?

Add R2N, R2DH #153

Conversation

MohamedLaghdafHABIBOULLAH commented Sep 12, 2024

dpo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohamedLaghdafHABIBOULLAH commented Sep 17, 2024

MohamedLaghdafHABIBOULLAH commented Sep 17, 2024

dpo commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohamedLaghdafHABIBOULLAH Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohamedLaghdafHABIBOULLAH Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

MohamedLaghdafHABIBOULLAH Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohamedLaghdafHABIBOULLAH commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxenceGollier commented Dec 19, 2024

dpo commented Dec 23, 2024

dpo commented Jan 9, 2025

dpo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxenceGollier commented Jan 10, 2025

MohamedLaghdafHABIBOULLAH commented Jan 10, 2025

dpo commented Jan 10, 2025

Choose a reason for hiding this comment

MohamedLaghdafHABIBOULLAH Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohamedLaghdafHABIBOULLAH Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

MohamedLaghdafHABIBOULLAH commented Jan 14, 2025

MohamedLaghdafHABIBOULLAH Oct 31, 2024 •

edited

Loading

MohamedLaghdafHABIBOULLAH Nov 1, 2024 •

edited

Loading

MohamedLaghdafHABIBOULLAH Nov 1, 2024 •

edited

Loading

MohamedLaghdafHABIBOULLAH Jan 13, 2025 •

edited

Loading

MohamedLaghdafHABIBOULLAH Jan 14, 2025 •

edited

Loading