CUQI-DTU / CUQIpy

https://cuqi-dtu.github.io/CUQIpy/
Apache License 2.0
46 stars 9 forks source link

Add ADMM + parameter reordering FISTA #553

Closed jeverink closed 2 weeks ago

jeverink commented 1 month ago

By request, sub PR from https://github.com/CUQI-DTU/CUQIpy/pull/431

Added ADMM solver

closes #562

jeverink commented 1 month ago

Thanks for all the feedback @nabriis.

I have now incorporated some of the feedback and like to have some feedback on some of the open conversations. Regarding the general cleaning up of the solver module, many of the comments also hold for many, if not all, of the other solvers. What do you think about making those a separate issue, to be cleaned up in a separate PR after this one? That might also help with keeping the pace up with all the smaller PRs to come.

The general issues your commented on summed up:

I also just noted that the two tests that fail seem to be are regarding sampling from the RegularizedGaussian/GMRF, which should not have been effected by the changes of this code. The only related change in this PR is a reordering of the parameters of FISTA (problem specific before algorithmic specific), but that should not cause these numerical issues. Do you see what might have gone wrong?

jeverink commented 3 weeks ago

Thanks for the help @nabriis, I have now fixed the test (I accidentally copy-pasted that one line that broke it all), and made some more final small improvements.

jeverink commented 2 weeks ago

Thanks for the feedback @amal-ghamdi, I have now updated the documentation and changed some of the variable names to be more inline with the reference article.