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

Add ADMM + parameter reordering FISTA #553

Merged

Conversation

jeverink
Copy link
Collaborator

@jeverink jeverink commented Oct 16, 2024

By request, sub PR from #431

Added ADMM solver

closes #562

@amal-ghamdi amal-ghamdi requested a review from nabriis October 21, 2024 06:43
Copy link
Collaborator

@nabriis nabriis 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 for splitting the PR into these smaller parts @jeverink!

I appreciate you adding this ADMM solver, including the adaptive scheme.

I know the current solver module is a bit lackluster with respect to code quality, but I was thinking that this PR can start moving the overall state of the solver module in a better direction. Therefore I added quite a bit of comments and suggestions.

Do not hesitate to reach out if you have questions on the comments.

cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Show resolved Hide resolved
cuqi/solver/_solver.py Show resolved Hide resolved
tests/test_solver.py Outdated Show resolved Hide resolved
@jeverink
Copy link
Collaborator Author

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:

  • Renaming callable f(x,*args) to callable with arguments (x, *args), possibly with extra information regarding the arguments.
  • Default parameter for initial guess x0?
  • Making most, if not all, attributes of the solvers private.
  • Docstring for the solve methods.
  • Guarding statements for inputs.

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?

@nabriis nabriis self-requested a review November 3, 2024 21:29
Copy link
Collaborator

@nabriis nabriis 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 @jeverink. I agree with your comments on code style etc. I created 3 issues. I only have a few comments left related to the code. I think we are nearly there! The refactor of the pre-processing was really helpful for readability. Thank you for adding the docstrings also!

cuqi/solver/_solver.py Outdated Show resolved Hide resolved
tests/test_solver.py Outdated Show resolved Hide resolved
@jeverink
Copy link
Collaborator Author

jeverink commented Nov 4, 2024

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.

Copy link
Collaborator

@nabriis nabriis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jeverink. LGTM!

Copy link
Contributor

@amal-ghamdi amal-ghamdi 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 @jeverink for your contribution, great work!

I made a few comments, please feel free to address them as you see fit and let me know if any of my comments are unclear.

cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Show resolved Hide resolved
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
cuqi/solver/_solver.py Show resolved Hide resolved
cuqi/solver/_solver.py Show resolved Hide resolved
@jeverink
Copy link
Collaborator Author

jeverink commented Nov 6, 2024

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.

@nabriis nabriis requested a review from amal-ghamdi November 7, 2024 15:38
cuqi/solver/_solver.py Outdated Show resolved Hide resolved
Copy link
Contributor

@amal-ghamdi amal-ghamdi 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 @jeverink! it looks great. I added one line suggestion in the description of the optimization functional and one comment about the documentation of the penalty_parameter for your consideration.
I am approving the PR.

@amal-ghamdi amal-ghamdi merged commit 1757009 into CUQI-DTU:main Nov 8, 2024
6 checks passed
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.

adding ADMM solver
3 participants