-
Notifications
You must be signed in to change notification settings - Fork 10
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
Reuse Arpack in order to compute norm of Bk #159
base: master
Are you sure you want to change the base?
Reuse Arpack in order to compute norm of Bk #159
Conversation
Please hold off on merging for now, as some tests are affected by #42 . @nathanemac you can try to merge this PR locally with R2N in order to use ADNLPModels |
@@ -130,7 +130,8 @@ function LMTR( | |||
JdFk = similar(Fk) # temporary storage | |||
Jt_Fk = similar(∇fk) # temporary storage | |||
|
|||
σmax = opnorm(Jk) | |||
σmax, found_σ = opnorm(Jk) | |||
found_σ || error("operator norm computation failed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a backup plan if the computation fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on understanding why it may not work ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@MohamedLaghdafHABIBOULLAH, using
|
Co-authored-by: Dominique <[email protected]>
@nathanemac thanks for the confirmation, we believe that Arrack may be more accurate than TSVD, but still have some bugs... |
Use alphabetical order for external dependencies and remove unused variables.
Enhance robustness of Arpack usage by catching NCV-related errors Catch the XYAUPD_Exception error in Arpack calls and dynamically increase NCV as needed to handle convergence issues. This modification addresses instability in LSR1 computations due to insufficient NCV.
This PR should address the problem encountered in this issue #42 |
while !(have_eig || attempt >= max_attempts) | ||
attempt += 1 | ||
try | ||
# Perform eigendecomposition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Perform eigendecomposition | |
# Estimate largest eigenvalue in absolute value |
catch e | ||
if occursin("XYAUPD_Exception", string(e)) | ||
@warn "Arpack error: $e. Increasing NCV to $ncv and retrying." | ||
ncv = min(2 * ncv, n) # Increase NCV but don't exceed matrix size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we still get an error when ncv == n
, we should abort.
while !(have_svd || attempt >= max_attempts) | ||
attempt += 1 | ||
try | ||
# Perform singular value decomposition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Perform singular value decomposition | |
# Estimate largest singular value |
Just wondering, @MohamedLaghdafHABIBOULLAH, is it possible to use the frobenius norm instead of the operator norm for your algorithm ? |
@MaxenceGollier yes one could and it has been discussed in the paper and in this issue #133 , however I'm afraid that the solver will become slower as the step size is related to 1/|B_k|, in this case the Frobenius norm might represent a loose bound on the operator norm or the largest eig norm. |
You can’t compute the Frobenius norm of a quasi-Newton operator without a lot of work. |
@MohamedLaghdafHABIBOULLAH can we add a wrapped allocs unit test to be sure that opnorm does not allocate as well ? |
Based on this commit 48e6f01#diff-72ed386c2a0cd1d23c0968297e70023ed98c22490d146dd89fc91f48369bad4dL7