Skip to content

Conversation

@MohamedLaghdafHABIBOULLAH
Copy link
Collaborator

Fix this 252

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds tracking of sigma_cauchy and scp_norm statistics to solver output across all trust-region and regularization-based solvers in the package. These statistics provide important diagnostic information about the solver's behavior at each iteration.

Key changes:

  • Addition of sigma_cauchy (reciprocal of step size parameter) tracking
  • Addition of scp_norm (norm of the proximal gradient step) tracking
  • Consistent placement of these statistics after they are computed in each solver

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/TR_alg.jl Added sigma_cauchy and scp_norm statistics tracking in the TR solver at initialization and main iteration loop
src/TRDH_alg.jl Added sigma_cauchy and scp_norm statistics tracking in the TRDH solver for both reduce_TR branches
src/R2_alg.jl Added scp_norm statistics tracking in the R2 solver at initialization and main iteration loop
src/R2N.jl Added scp_norm statistics tracking in the R2N solver at initialization and main iteration loop
src/R2DH.jl Added scp_norm statistics tracking and moved sigma_cauchy to after ν₁ is computed in the R2DH solver
src/LM_alg.jl Added sigma_cauchy and scp_norm statistics tracking in the Levenberg-Marquardt solver at initialization and main iteration loop
src/LMTR_alg.jl Added sigma_cauchy and scp_norm statistics tracking in the LMTR solver at initialization and main iteration loop

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codecov
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.67%. Comparing base (e0f214d) to head (850248d).
⚠️ Report is 248 commits behind head on master.

Files with missing lines Patch % Lines
src/TRDH_alg.jl 33.33% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #253       +/-   ##
===========================================
+ Coverage   61.53%   84.67%   +23.14%     
===========================================
  Files          11       13        +2     
  Lines        1292     1651      +359     
===========================================
+ Hits          795     1398      +603     
+ Misses        497      253      -244     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dpo
Copy link
Member

dpo commented Oct 17, 2025

Would it also be useful to report $\Vert s_k \Vert$ in addition to $\Vert s_{k,cp} \Vert$? PANOC doesn't compute an $s_{k,cp}$, and it could be that $\Vert s_k \Vert < \Vert s_{k,cp} \Vert$.

@MohamedLaghdafHABIBOULLAH
Copy link
Collaborator Author

Would it also be useful to report ‖ s k ‖ in addition to ‖ s k , c p ‖ ? PANOC doesn't compute an s k , c p , and it could be that ‖ s k ‖ < ‖ s k , c p ‖ .

I am not sure if the use of ‖ s k ‖ would be suitable for a stationary measure as in R2N we do not necessarily have ‖ s k ‖ < ‖ s k , c p ‖!

Copy link
Collaborator

@MaxenceGollier MaxenceGollier left a comment

Choose a reason for hiding this comment

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

Excellent,

I wonder if scp_norm is a good name though. Since we have a sigma_cauchy, wouldn't it be more appropriate to rename scp_norm as step_cauchy_norm or cauchy_point_norm ? Just an idea we might as well just keep scp_norm.

@dpo dpo mentioned this pull request Dec 1, 2025
@MohamedLaghdafHABIBOULLAH
Copy link
Collaborator Author

@MaxenceGollier I think the name is correct, what do you think @dpo?

Copy link
Collaborator

@MaxenceGollier MaxenceGollier left a comment

Choose a reason for hiding this comment

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

Maybe add the .gitignore in a separate PR ?

The rest looks good to me.


@. mν∇fk = - ν * ∇fk
prox!(s, ψ, mν∇fk, ν)
set_solver_specific!(stats, :scp_norm, norm(s))
Copy link
Member

Choose a reason for hiding this comment

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

This norm is already computed. Don't compute it twice.

Copy link
Collaborator Author

@MohamedLaghdafHABIBOULLAH MohamedLaghdafHABIBOULLAH Dec 6, 2025

Choose a reason for hiding this comment

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

Here, $$s$$ actually refers to $$s_{cp}$$, whereas the other norm being computed corresponds to the step $$s_{k}$$ used in the update $$x_{k+1} = x_{k} + s_{k}.$$

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see the issue when we consider R2, R2DH or TRDH

@MaxenceGollier
Copy link
Collaborator

Can you make the corrections please ? @MohamedLaghdafHABIBOULLAH, we are waiting for this to be merged before releasing the package :)

Co-authored-by: Maxence Gollier <[email protected]>
@MohamedLaghdafHABIBOULLAH
Copy link
Collaborator Author

MohamedLaghdafHABIBOULLAH commented Dec 6, 2025

@MaxenceGollier @dpo I think we can keep this as a draft for now and revisit it after the release. The goal here is to store the norm of $$s$$ intelligently, but $$s$$ itself changes because $$s_{cp}$$ is overwritten by $$s$$. This is not the case for R2DH and R2, where $$s_{cp}$$ is already considered as $$s$$, and for TRDH it is more delicate since it depends on the reduce_tr argument.

@MohamedLaghdafHABIBOULLAH
Copy link
Collaborator Author

There seems to be a problem related to the augmented Lagrangian method:

Test Summary:        | Error  Total  Time
Augmented Lagrangian |     1      1  9.1s
RNG of the outermost testset: Xoshiro(0xdb2fa90498613fdf, 0x48d73dc42d195740, 0x8c49bc52dc8a77ea, 0x1911b814c02405e8, 0x22a21880af5dc689)
ERROR: LoadError: Some tests did not pass: 0 passed, 0 failed, 1 errored, 0 broken.
in expression starting at /Users/laghdafhacen/Documents/GitHub_PFE/RegularizedOptimization.jl/test/test_AL.jl:4
in expression starting at /Users/laghdafhacen/Documents/GitHub_PFE/RegularizedOptimization.jl/test/runtests.jl:21
ERROR: Package RegularizedOptimization errored during testing

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