Skip to content

Conversation

MohamedLaghdafHABIBOULLAH
Copy link
Collaborator

Fix this 252

Copy link
Contributor

@Copilot 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.

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