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 proximal ADMM solvers #371

Merged
merged 88 commits into from
Dec 13, 2022
Merged

Add proximal ADMM solvers #371

merged 88 commits into from
Dec 13, 2022

Conversation

bwohlberg
Copy link
Collaborator

Add proximal ADMM solvers.

@bwohlberg bwohlberg added the enhancement New feature or request label Dec 9, 2022
@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #371 (3184b29) into main (fd26f49) will decrease coverage by 0.06%.
The diff coverage is 94.09%.

@@            Coverage Diff             @@
##             main     #371      +/-   ##
==========================================
- Coverage   94.15%   94.08%   -0.07%     
==========================================
  Files          57       61       +4     
  Lines        3406     3824     +418     
==========================================
+ Hits         3207     3598     +391     
- Misses        199      226      +27     
Flag Coverage Δ
unittests 94.08% <94.09%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
scico/optimize/_padmm.py 92.35% <92.35%> (ø)
scico/denoiser.py 88.57% <95.00%> (+0.79%) ⬆️
scico/function.py 96.49% <96.49%> (ø)
scico/linop/_util.py 100.00% <100.00%> (ø)
scico/optimize/__init__.py 100.00% <100.00%> (ø)
scico/optimize/_primaldual.py 100.00% <100.00%> (ø)
scico/__init__.py 100.00% <0.00%> (ø)
scico/util.py 92.46% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Michael-T-McCann
Copy link
Contributor

"""
For reasons that are not entirely clear, the first step of the first-run
solver is much slower than the following steps. Perform a preliminary
solver step, the result of which is discarded, to avoid this bias in the
timing results.
"""

This is probably caused by the operators being jitted the first time they are called.

Copy link
Contributor

@Michael-T-McCann Michael-T-McCann left a comment

Choose a reason for hiding this comment

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

Couple suggestions and at least one typo.

examples/scripts/denoise_tv_multi.py Show resolved Hide resolved
examples/scripts/denoise_tv_multi.py Show resolved Hide resolved
examples/scripts/denoise_tv_multi.py Show resolved Hide resolved
snp.vstack(
(hist_admm.Objective, hist_ladmm.Objective, hist_padmm.Objective, hist_pdhg.Objective)
).T,
snp.vstack((hist_admm.Time, hist_ladmm.Time, hist_padmm.Time, hist_pdhg.Time)).T,
ptyp="semilogy",
title="Objective function",
xlbl="Time (s)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the curves still appear to a large horizontal shift, as if there is a lot of compute happening before iteration 1. Suggestion: add a comment on this if you have a reason why.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not occurring when the script is run locally without a GPU. Will investigate.

examples/scripts/denoise_cplx_tv_nlpadmm.py Outdated Show resolved Hide resolved
examples/scripts/denoise_cplx_tv_nlpadmm.py Outdated Show resolved Hide resolved
examples/scripts/denoise_cplx_tv_nlpadmm.py Outdated Show resolved Hide resolved
examples/scripts/denoise_cplx_tv_pdhg.py Show resolved Hide resolved
scico/optimize/_padmm.py Outdated Show resolved Hide resolved
scico/optimize/_padmm.py Show resolved Hide resolved
examples/scripts/denoise_cplx_tv_nlpadmm.py Outdated Show resolved Hide resolved
examples/scripts/denoise_cplx_tv_nlpadmm.py Outdated Show resolved Hide resolved
examples/scripts/denoise_cplx_tv_nlpadmm.py Outdated Show resolved Hide resolved
2. Proximal ADMM has similar performance to PDHG with respect to iterations,
but is slightly inferior with respect to time.
3. ADMM greatly outperforms Linearized ADMM with respect to iterations.
4. ADMM slightly outperforms Linearized ADMM with respect to time. This is
possible because the ADMM $\mathbf{x}$-update can be solved relatively
cheaply, with only 2 CG iterations. If more CG iterations were required,
the time comparison would be favorable to Linearized ADMM.
Copy link
Contributor

Choose a reason for hiding this comment

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

not clear, if more CG iterations were required which one would be superior to which other one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what's not clear. Does "favorable" seem ambiguous?

@bwohlberg bwohlberg merged commit d05ff9c into main Dec 13, 2022
@bwohlberg bwohlberg deleted the brendt/padmm-ext2 branch December 13, 2022 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants