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

fix regularization in CRAIG #171

Merged
merged 1 commit into from
Aug 14, 2020

Conversation

amontoison
Copy link
Member

@amontoison amontoison commented Jan 7, 2020

I introduced a mistake in CRAIG with #104.

 [  γ    λ ] [ c₂   s₂ ] = [  δ    0 ]
 [  0    0 ] [ s₂  -c₂ ]   [  0    0 ]
 c₂, s₂, δ = sym_givens(γ, λ)

Or it's γ that we want to zero out and not λ.

 [  γ    λ ] [ -c₂   s₂ ] = [  0    δ ]
 [  0    0 ] [  s₂   c₂ ]   [  0    0 ]
 c₂, s₂, δ = sym_givens(λ, γ)

The commit fix this error, add a boolean parameter sqd + some tests for regularized problems and saddle-point / symmetric quasi-definite systems.

Docstrings have been updated too, it's Ax + λs = b that is solved when λ > 0 and not
Ax + √λs = b.

@codecov
Copy link

codecov bot commented Jan 7, 2020

Codecov Report

Merging #171 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #171   +/-   ##
=======================================
  Coverage   97.20%   97.20%           
=======================================
  Files          28       28           
  Lines        2503     2504    +1     
=======================================
+ Hits         2433     2434    +1     
  Misses         70       70           
Impacted Files Coverage Δ
src/craig.jl 97.08% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6c9bf4...c92b596. Read the comment docs.

@coveralls
Copy link

coveralls commented Jan 7, 2020

Coverage Status

Coverage increased (+0.001%) to 97.204% when pulling c92b596 on amontoison:regularization-craig into d6c9bf4 on JuliaSmoothOptimizers:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 97.199% when pulling 2072e5a on amontoison:regularization-craig into adc5bec on JuliaSmoothOptimizers:master.

@amontoison amontoison force-pushed the regularization-craig branch 3 times, most recently from f46b0bb to a50d912 Compare January 8, 2020 20:14
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.

Thank you. Here are a few simple comments. I'll have to check the other details carefully when I get back.

src/craig.jl Outdated Show resolved Hide resolved
src/craigmr.jl Outdated Show resolved Hide resolved
@amontoison amontoison force-pushed the regularization-craig branch 2 times, most recently from 3027753 to 1933ee9 Compare March 26, 2020 19:31
@amontoison amontoison force-pushed the regularization-craig branch 2 times, most recently from f34236e to 9b24b32 Compare April 14, 2020 18:37
@amontoison amontoison mentioned this pull request Apr 15, 2020
@amontoison amontoison changed the title Regularization in CRAIG and CRAIGMR fix regularization in CRAIG Aug 2, 2020
@amontoison
Copy link
Member Author

I updated this pull request, it only concerns CRAIG now. It fixes the computation of a Givens reflection during the regularization.

@dpo dpo merged commit 232175b into JuliaSmoothOptimizers:master Aug 14, 2020
@dpo
Copy link
Member

dpo commented Aug 14, 2020

Thank you.

@amontoison amontoison deleted the regularization-craig branch August 4, 2021 19:55
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