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

Pdhg strongly convex #1030

Merged
merged 99 commits into from
Nov 19, 2021
Merged

Pdhg strongly convex #1030

merged 99 commits into from
Nov 19, 2021

Conversation

epapoutsellis
Copy link
Contributor

@epapoutsellis epapoutsellis commented Oct 26, 2021

This PR adds primal OR dual acceleration for the PDHG algorithm in the case where the function f^{*} or g is strongly convex. These are **kwargs** arguments in the PDHG algorithm,i.e., gamma_g for the strongly convex constant of the function g (primal acceleration) and gamma_fconj for the strongly convex constant of the convex conjugate of f (dual acceleration). In particular:

  • Checks if the strongly convex constants are positive Numbers.
  • Always raises a warning for the users to be sure that the number is correct and related to the strongly convex property of the function.
  • Creates a new method update_step_sizes which is called at the end of the update method. Therefore, the step-sizes change in every iteration, according to the specific case, f^{*} or g being strongly convex . If gamma_g or gamma_fconj are None, the step-sizes remain unchanged.
  • Error is raised if the user passes strongly convex constants for both functions. This adaptive rule is not implemented at the moment.
  • Creates a new method set_step_sizes to check the correct type of the primal-dual step-sizes, sigma and tau and set the default scalar values.
  • Check if sigma, tau are positive Numbers, if they are array-like objects with the correct shape determined by the operator. If the user passes sigma and tau with the correct shape, there is no guarantee that the algorithm will converge unless it follows Lemma 2.
    - If the shape is correct, the PDHG algorithm will not run, unless use_axpby=False. Fixed by axpby blackdatacontainer fix #1080 .
  • Raises a warning if the convergence condition for the case of scalar step-sizes is not satisfied.

Note The adaptive rule for primal or dual acceleration is described in Algorithm 2.

  1. A denoising example can be found here where the strong convexity property comes from the function L2NormSquared.

  2. A tomography example can be found here where the strong convexity property comes from the convex conjugate of the BlockFunction f. Not that the ProjectionOperator is with device=cpu to avoid any mismatch on the adjoint.

@epapoutsellis epapoutsellis requested review from gfardell, paskino and jakobsj and removed request for gfardell October 26, 2021 15:49
Copy link
Contributor

@paskino paskino left a comment

Choose a reason for hiding this comment

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

I guess we need to:

  1. document the additional variables gamma_g and gamma_fconj in the docstring
  2. add unittests (basic, not just one full denoising example). Something like set up a PDHG without the gammas, do 1 iteration and check that the sigma and tau remain the same. Set up a PDHG with the gammas, do 1 iteration and check that the sigma and tau have been changed to what you expect.

@@ -17,11 +17,12 @@

from cil.optimisation.algorithms import Algorithm
import warnings

import numpy
Copy link
Contributor

Choose a reason for hiding this comment

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

import numpy as np

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@epapoutsellis epapoutsellis requested a review from paskino October 28, 2021 12:40
@jakobsj
Copy link
Contributor

jakobsj commented Oct 28, 2021

Great to get a strong convexity version in but I'm not sure about the parameters being given as additional parameters to PDHG. Wouldn't they belong more naturally as attributes of the Functions, similar to Lipschitz? With default being None and automatically set on initialising the Function whenever possible? Then PDHG would check the f and g Functions for these parameters and if different from None the strongly convex version of the algorithm would be used.

Then the user wouldn't need to worry about understanding what these parameters are, computing them (how would they do that?) and providing them as input.

Not sure if this approach has been discussed?

@paskino
Copy link
Contributor

paskino commented Oct 29, 2021

@jakobsj 's got a great point here. If we put the gammas as properties of Functions we will be able to:

  1. immediately provide a working implementation by manually setting such variables in the function
  2. such implementation can be further developed when we figure out how to compute the gammas and add this logic to the relevant function classes.

@epapoutsellis
Copy link
Contributor Author

epapoutsellis commented Oct 29, 2021

That was my initial plan, but we decided to not follow this route. Basically, we cannot do it atm. As the convex_conjugate returns a number and not a function.

In this example, we use the strongly convexity case of PDHG, when the conjugate of the BlockFunction , i.e., separable function is strongly convex. That is the sum of the convex_conjugate of L2NormSquared and the convex_conjugate of the MixedL21Norm.

If you run the example above, you will see the following figure (label is wrong, **should be Strongly Convex (fconj) **)

pdgap

@jakobsj
Copy link
Contributor

jakobsj commented Oct 29, 2021 via email

@paskino paskino added this to the version 21.3 milestone Nov 1, 2021
@epapoutsellis
Copy link
Contributor Author

@gfardell I prefer to have the methods update, set_step_sizes, update_step_sizes, update_objective in the docs and not use _ because I believe they are important and if the user does not want to read the docs and see what is going on in these methods, can click on source and have a look at the code.

Copy link
Member

@gfardell gfardell left a comment

Choose a reason for hiding this comment

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

I think the example code is my biggest issue. It's unmaintainable as it is, and worse than having the code snippets in __main__ as we used to. Can the examples be restricted to how to set up, modify and use the class. We have CIL demos for full CIL usage.

Wrappers/Python/cil/optimisation/algorithms/PDHG.py Outdated Show resolved Hide resolved
Wrappers/Python/cil/optimisation/algorithms/PDHG.py Outdated Show resolved Hide resolved
@gfardell
Copy link
Member

@gfardell I prefer to have the methods update, set_step_sizes, update_step_sizes, update_objective in the docs and not use _ because I believe they are important and if the user does not want to read the docs and see what is going on in these methods, can click on source and have a look at the code.

I don't think they'll know the difference between set_step_sizes and update_step_sizes. if one is marked as _ then they can see it (you can even include it in the docs if you want), but they won't modify it. But we have not used this elsewhere in algorithms so we can leave it for now.

@epapoutsellis
Copy link
Contributor Author

I removed the Example section and refer to the CIL-Demos repo and the papers. The default sigma and tau step-sizes are described explicitly in the beginning of the docs.

- By default, the step sizes :math:`\sigma` and :math:`\tau` are positive scalars and defined as below:
* If ``sigma`` is ``None`` and ``tau`` is ``None``:
.. math::
\sigma = \frac{1}{\|K\|}, \tau = \frac{1}{\|K\|}
* If ``tau`` is ``None``:
.. math::
\tau = \frac{1}{\sigma\|K\|^{2}}
* If ``sigma`` is ``None``:
.. math::
\sigma = \frac{1}{\tau\|K\|^{2}}


#check if sigma, tau are None
pdhg = PDHG(f=f, g=g, operator=operator, max_iteration=10)
self.assertEqual(pdhg.sigma, 1./operator.norm())
Copy link
Contributor

Choose a reason for hiding this comment

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

use assertAlmostEqual


self.set_gamma_g(kwargs.get('gamma_g', None))
self.set_gamma_fconj(kwargs.get('gamma_fconj', None))
if self.gamma_g is not None and self.gamma_fconj is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

move this check into the setter

Copy link
Contributor

@paskino paskino left a comment

Choose a reason for hiding this comment

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

A few minor changes

@paskino paskino merged commit 42a030c into master Nov 19, 2021
@paskino paskino deleted the PDHG_strongly_convex branch November 19, 2021 14:35
@epapoutsellis epapoutsellis mentioned this pull request May 18, 2022
10 tasks
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.

4 participants