Fix scaling bug in CG

[tbalke]

@bwohlberg @lukepfister
Kept the scale factor in the Hessian the same so that it truly is still the Hessian but removed the other scale factor.

[lukepfister]

The ADMM quadratic tests are failing now. Can you look into that?

[tbalke]

The ADMM quadratic tests are failing now. Can you look into that?

Interesting. It really is because of the change I made. (Assumed it was something else because it is just a scaler taken out.)

I'll look into it.

[tbalke]

Amended the commit. Tests passing now.

Both the scale and the 2.0 factors needed to be removed.

[tbalke]

I think it is ready for rebase and merge now.

[bwohlberg]

Resolves #38

[bwohlberg]

Can we add a test that would catch the bug?

[lukepfister]

Change the loss scale to something like .3333 in the existing ADMM quadratic tests and that should do the trick

…

On Thu, Oct 14, 2021 at 8:59 PM Brendt Wohlberg ***@***.***> wrote: Can we add a test that would catch the bug? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJHYQCLOWHT2AJUKAYTEQTUG6KHPANCNFSM5F6RXKRA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[bwohlberg]

Change the loss scale to something like .3333 in the existing ADMM quadratic tests and that should do the trick

That doesn't do the trick, unfortunately. See branch brendt/admm-quadratic-test, which was made off the branch for this PR. The modified tests pass for GenericSubproblemSolver but they fail for LinearSubproblemSolver, so either (a) this PR doesn't resolve the problem, or (b) there's something wrong with the tests. If the tests and fix are correct, the tests involving GenericSubproblemSolver should fail for the main branch but pass in the branch for this PR.

[tbalke]

See branch brendt/admm-quadratic-test, which was made off the branch for this PR

No, brendt/admm-quadratic-test branches of a different commit.

When using the fix from this PR most of the tests from brendt/admm-quadratic-test pass again, including the one with LinearSubproblemSolver. (Not all tests pass though)

scico/test/test_admm.py::TestReal::test_admm_generic PASSED                                                                                   [ 16%]
scico/test/test_admm.py::TestReal::test_admm_quadratic_scico PASSED                                                                           [ 33%]
scico/test/test_admm.py::TestReal::test_admm_quadratic_jax PASSED                                                                             [ 50%]
scico/test/test_admm.py::TestComplex::test_admm_generic FAILED                                                                                [ 66%]
scico/test/test_admm.py::TestComplex::test_admm_quadratic PASSED                                                                              [ 83%]
scico/test/test_admm.py::TestCircularConvolveSolve::test_admm PASSED                                                                          [100%]

[bwohlberg]

No, brendt/admm-quadratic-test branches of a different commit.

You're right, I branched it off the other tbalke/... branch by mistake.

When using the fix from this PR most of the tests from brendt/admm-quadratic-test pass again, including the one with LinearSubproblemSolver. (Not all tests pass though)

All tests pass for me now that I recreated my branch as a branch off of tbalke/cgfix.

scico/test/test_admm.py::TestComplex::test_admm_generic FAILED

Could you check the detailed log on this test? Perhaps it's failing because the accuracy tolerance is a bit too tight?

[tbalke]

All tests pass for me now that I recreated my branch as a branch off of tbalke/cgfix.

For me, too, all tests are passing now. Thanks!