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

Update algorithms interface, update docs #62

Merged
merged 10 commits into from
Jan 31, 2022

Conversation

lostella
Copy link
Member

@lostella lostella commented Jan 30, 2022

This PR touches lots things, but the changes are not that many, with several concerning documentation. However, a few breaking changes are there. All of the changes were done working backwards from the documentation process, trying to simplify and uniformize the interface to algorithms.

Breaking changes

  • All algorithms now require the initial iterate as keyword arguments (before it was positional). Therefore now all arguments are keyword arguments.
  • DavisYin now has f as smooth term (was h) to align it with all other algorithms; the smoothness constant was renamed to Lf as a consequence.
  • DRLS now uses mf as strong convexity parameter (was muf).
  • SFISTA arguments were uniformized to other algorithms: x0 is the initial iterate, g is the proximable term, mf is the convexity modulus of f.

Other changes

  • Introduced the IterativeAlgorithm type, which encodes at once how to construct and iterate a given iterator type, with stopping criterion and verbosity and so on; specific algorithms construct an object of this type now, with a particular iterator type, default stopping criterion (which can now be replaced with anything if one wants, not just tuned via the tolerance) and default values for other options.
  • Reworked PANOC, PANOCplus, and ZeroFPR iterate method to make it type stable (a new state is always returned, never nothing).

Documentation changes

  • Reorganized and updated material.
  • Fixed docstrings.
  • Added docstrings for the “solver” interface to algorithms.
  • Switched to Plots for doing plots, instead of Gadfly.

Tests

Except for the changes to the algorithms interface (described above), test scripts are unchanged.

@codecov
Copy link

codecov bot commented Jan 30, 2022

Codecov Report

Merging #62 (20dda99) into master (3cd2cca) will decrease coverage by 0.74%.
The diff coverage is 95.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
- Coverage   89.33%   88.59%   -0.75%     
==========================================
  Files          22       22              
  Lines         966      894      -72     
==========================================
- Hits          863      792      -71     
+ Misses        103      102       -1     
Impacted Files Coverage Δ
src/algorithms/li_lin.jl 75.55% <80.00%> (-3.30%) ⬇️
src/algorithms/primal_dual.jl 61.73% <85.71%> (-2.20%) ⬇️
src/algorithms/drls.jl 94.52% <90.00%> (-0.48%) ⬇️
src/algorithms/sfista.jl 83.33% <90.90%> (ø)
src/algorithms/davis_yin.jl 92.30% <92.85%> (-1.64%) ⬇️
src/ProximalAlgorithms.jl 90.00% <100.00%> (+90.00%) ⬆️
src/algorithms/douglas_rachford.jl 93.33% <100.00%> (-2.32%) ⬇️
src/algorithms/fast_forward_backward.jl 96.96% <100.00%> (-0.54%) ⬇️
src/algorithms/forward_backward.jl 96.29% <100.00%> (-0.77%) ⬇️
src/algorithms/panoc.jl 97.70% <100.00%> (-0.13%) ⬇️
... and 3 more

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 3cd2cca...20dda99. Read the comment docs.

@lostella lostella marked this pull request as ready for review January 30, 2022 08:18
@lostella
Copy link
Member Author

@aldma since you contributed PANOCplus, I would kindly ask you to take a look to the changes I’m doing to it here (see PR description), whenever you are able to, and confirm that they look OK.

Feel free to focus on the changes to PANOCplus, of course :-)

In particular, I’m changing the behavior when the maximum number of backtracking steps on tau is exceeded, to have the “nominal” forward-backward step accepted instead of the iteration halting.

@lostella
Copy link
Member Author

lostella commented Jan 30, 2022

@wwkong I’m proposing some breaking changes to the SFISTA method you contributed, to uniformize the naming across algorithms: the proximable term h becomes g, the initial iterate y0 becomes x0, the convexity modulus of f becomes mf. So this PR is simply renaming things, and the algorithm is unaffected. Please feel free to look into the changes (only the SFISTA ones!) and confirm I didn’t do anything silly there :-)

@aldma
Copy link
Collaborator

aldma commented Jan 30, 2022

@lostella Your changes to PANOCplus look good. Nice refactoring and docs, indeed!

@lostella
Copy link
Member Author

Merging, I’ll address further comments separately, if there are any 🙃

@lostella lostella merged commit 24d0e9e into JuliaFirstOrder:master Jan 31, 2022
@lostella lostella deleted the algorithm-interface branch January 31, 2022 19:39
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.

2 participants