ProximalAlgorithms.jl icon indicating copy to clipboard operation
ProximalAlgorithms.jl copied to clipboard

StructuredOptimization-related changes + new algorithms

Open hakkelt opened this issue 1 month ago • 4 comments

This PR is part of the effort to make StructuredOptimization as general as possible (see the PR on OperatorCore: https://github.com/JuliaFirstOrder/ProximalCore.jl/pull/5).

Changes:

  • new get_assumptions function defined for all algorithms that allows querying the requirements for the algorithm's parameters (e.g., f must be is_smooth, g must be is_proximable). The function returns a dict-like object that has the names of the parameters as keys and a tuple of functions that are expected to return true for all allowed inputs as values. The concept is to enable automatic decisions on how to feed functions of the ProximalOperators package and operators of AbstractOperators to the algorithms.
  • Separate default_iteration_summary function from default_display function and allow overriding them separately through the general IterativeAlgorithm interface.
  • Improve the default display functionality by showing the header and automatically figuring out the optimal column width (it implements some of the wishes in #70 ).
  • Add override_parameters function that allows overwriting values of fields in IterativeAlgorithm struct by creating a copy with changed values. I use this in a high-level package to inject default maxit and atol values into the user-provided algorithm.
  • Introduce two new algorithms:
    • Alternating Directions of Multipliers Method (ADMM)
    • (Preconditioned) Conjugate Gradient (CG) -- I know that this is not a proximal algorithm, but ADMM uses it internally, so it seemed logical to also implement the full interface of ProximalAlgorithms.
  • Preallocate more in DavisYin, DouglasRachford, and FastForwardBackward
  • Minor fixes in docstrings

This PR contains no breaking changes (as far as I know), only introduces new features.

hakkelt avatar Nov 11 '25 21:11 hakkelt

@hakkelt I see that you're attempting various changes in different packages, all motivated by StructuredOptimization. I think it might be fine to move some core definitions to ProximalCore, but I would encourage you to:

  • Keep PRs small and focused, since reviewing 3000 line changes is very hard, especially if the changes are not always related
  • Keep the changes that belong to StructuredOptimization, into StructuredOptimization. Not everything needs to be pushed "upstream" from the get go, necessarily. For example, could you not define assumptions for the algorithms in ProximalAlgorithms, directly in StructuredOptimization? This is where the assumptions are needed, so it should be fine to define them there, without introducing inter-package design choices too early. Once things work well, one can think of exposing these interfaces in more "core" packages, and put the definitions next to the types. This way the wider plan is easier to understand & review

lostella avatar Nov 12 '25 15:11 lostella

Thanks for the feedback, and sorry for the messy PRs. Would it help if I discard this PR and break the changes included in it into multiple smaller pull requests, submitting them one by one?

Concerning get_assumption: I'm ok with moving it to StructuredOptimization as it might too early have it here, but for maintainability, I would be glad if some machinery that allows querying is incorporated into AbstractAlgorithms.

hakkelt avatar Nov 13 '25 08:11 hakkelt

Instead of this large PR, I'll make multiple smaller, more focused PRs, starting with #101

hakkelt avatar Dec 02 '25 11:12 hakkelt

Thanks @hakkelt, I’ll take a look at it asap

lostella avatar Dec 02 '25 11:12 lostella