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

Add R2N, R2DH #153

Merged
merged 38 commits into from
Jan 17, 2025
Merged

Conversation

MohamedLaghdafHABIBOULLAH
Copy link
Contributor

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

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.

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

src/LM_alg.jl Outdated Show resolved Hide resolved
src/LM_alg.jl Outdated Show resolved Hide resolved
@MohamedLaghdafHABIBOULLAH MohamedLaghdafHABIBOULLAH force-pushed the R2N-R2DH branch 3 times, most recently from 4700798 to b6cd089 Compare September 17, 2024 05:43
@MohamedLaghdafHABIBOULLAH
Copy link
Contributor Author

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

@MohamedLaghdafHABIBOULLAH
Copy link
Contributor Author

@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
Copy link
Member

dpo commented Sep 17, 2024

@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 dpo closed this Sep 17, 2024
@dpo dpo reopened this Sep 17, 2024
@MohamedLaghdafHABIBOULLAH MohamedLaghdafHABIBOULLAH force-pushed the R2N-R2DH branch 2 times, most recently from acaad65 to 652701d Compare September 17, 2024 19:29
@MohamedLaghdafHABIBOULLAH MohamedLaghdafHABIBOULLAH changed the title Add R2N, R2DH and update LM Add R2N, R2DH Sep 27, 2024
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated
D.d .= summation ? D.d .+ σk : D.d .* σk
DNorm = norm(D.d, Inf)


Copy link
Member

Choose a reason for hiding this comment

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

Remove all duplicate blank lines.

src/R2DH.jl Outdated Show resolved Hide resolved
src/R2_alg.jl Outdated Show resolved Hide resolved
src/R2_alg.jl Outdated Show resolved Hide resolved
src/input_struct.jl Show resolved Hide resolved
src/R2N.jl Outdated Show resolved Hide resolved
src/R2N.jl Outdated Show resolved Hide resolved
src/R2N.jl Outdated Show resolved Hide resolved
src/R2N.jl Outdated Show resolved Hide resolved
src/R2N.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
@MohamedLaghdafHABIBOULLAH
Copy link
Contributor Author

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

src/R2DH.jl Outdated Show resolved Hide resolved
src/R2N.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2N.jl Outdated Show resolved Hide resolved
src/R2N.jl Show resolved Hide resolved
@MaxenceGollier
Copy link
Contributor

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
Copy link
Member

dpo commented Dec 23, 2024

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 Outdated Show resolved Hide resolved
@dpo
Copy link
Member

dpo commented Jan 9, 2025

@MohamedLaghdafHABIBOULLAH Please rebase this branch against main.

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.

@MohamedLaghdafHABIBOULLAH Let’s please finish this fast.

src/R2N.jl Outdated Show resolved Hide resolved
src/R2N.jl Outdated Show resolved Hide resolved
src/R2N.jl Outdated Show resolved Hide resolved
src/R2N.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
src/R2DH.jl Outdated Show resolved Hide resolved
@MaxenceGollier
Copy link
Contributor

@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]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
I will add (-Inf) condition in ShiftedProximalOperators as well
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Dominique <[email protected]>
Co-authored-by: Maxence Gollier <[email protected]>
Add a documentation for second calling form of R2DH
@MohamedLaghdafHABIBOULLAH
Copy link
Contributor Author

The tests with Julia 1 fail.

Just sync your fork @MohamedLaghdafHABIBOULLAH and tests should pass on Julia 1. The errors are on the R2 allocation test which where solved recently in #164

Now it should be better @MaxenceGollier.

@MaxenceGollier
Copy link
Contributor

The demo with TRDH keeps failing by the way, perhaps we should add a random seed to prevent it.

@MohamedLaghdafHABIBOULLAH
Copy link
Contributor Author

MohamedLaghdafHABIBOULLAH commented Jan 16, 2025

The demo with TRDH keeps failing by the way, perhaps we should add a random seed to prevent it.

You mean we need to change the seed?

@MohamedLaghdafHABIBOULLAH
Copy link
Contributor Author

demo-Julia-1-test

Even on the master branch, this demo test fails. Should we open an issue?

@dpo
Copy link
Member

dpo commented Jan 17, 2025

The demos will have to be overhauled completely after we remove FHist, etc., anyways.

@dpo dpo merged commit 356eef3 into JuliaSmoothOptimizers:master Jan 17, 2025
11 of 12 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