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

Spdhg with stochastic sampler #1644

Open
wants to merge 131 commits into
base: master
Choose a base branch
from

Conversation

MargaretDuff
Copy link
Member

@MargaretDuff MargaretDuff commented Jan 10, 2024

Describe your changes

  • Allow SPDHG to take a sampler either from our sampler class or any class with a next(self) function
  • Deprecated prob from SPDHG, taking the probabilities instead from the sampler class or from a new argument prob_weights, choosing the default [1/num_subsets]*num_subsets if one is not provided in either place.
  • Created two setters for the step sizes. set_step_sizes_from_ratio resets the step sizes if the user provides one/both/none of gamma and rho - note that this closes SPDHG gamma parameter is applied incorrectly  #1860. step_sizes_custom takes in one/both/none of sigma and tau allowing the user to use a custom sigma and tau with those not provided calculated from the defaults. Calculating sigma from tau probably needs checking with someone else.
  • Added a check_convergence function that checks self._sigma[i] * self._tau * self.norms[i]**2 <= self.prob_weights[i] for all i. This probably needs checking with someone else.
  • Deprecated the kwarg "norms" to be replaced by the set_norms method in BlockOperator: added a function to return a list of norms and the ability to set this list of norms BlockOperator: added a function to return a list of norms and the ability to set this list of norms BlockOperator: added a function to return a list of norms and the ability to set this list of norms  #1513.
  • Unit tests for SPDHG setters and convergence check
  • Fixes BlockOperator.domain_geometry().allocate() not compatible with in place calls to BlockOperator.direct  #1863

Describe any testing you have performed

Please add any demo scripts to CIL-Demos/misc/
Test with SPDHG https://github.com/TomographicImaging/CIL-Demos/blob/main/misc/testing_sampling_SPDHG.ipynb

Similar results gained for all samplers for SPDHG, with 10 subsets
image

With 80 subsets:
image

Link relevant issues

Part of the stochastic work plan. Closes #1575. Closes #1576. Closes #1500. Closes #1496

Checklist when you are ready to request a review

  • I have performed a self-review of my code
  • I have added docstrings in line with the guidance in the developer guide
  • I have implemented unit tests that cover any new or modified functionality
  • CHANGELOG.md has been updated with any functionality change
  • Request review from all relevant developers
  • Change pull request label to 'Waiting for review'

Contribution Notes

Please read and adhere to the developer guide and local patterns and conventions.

  • The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in CIL (the Work) under the terms and conditions of the Apache-2.0 License.
  • I confirm that the contribution does not violate any intellectual property rights of third parties

MargaretDuff and others added 30 commits August 2, 2023 10:36
Quick docstring

Signed-off-by: Margaret Duff <[email protected]>
@MargaretDuff
Copy link
Member Author

For reference:
PDHG parameters:

  • f (Function) – A convex function with a “simple” proximal method of its conjugate.

  • g (Function) – A convex function with a “simple” proximal.

  • operator (LinearOperator) – A Linear Operator.

  • sigma (positive float, or np.ndarray, DataContainer, BlockDataContainer, optional, default=None) – Step size for the dual problem.

  • tau (positive float, or np.ndarray, DataContainer, BlockDataContainer, optional, default=None) – Step size for the primal problem.

  • initial (DataContainer, optional, default=None) – Initial point for the PDHG algorithm.

  • gamma_g (positive float, optional, default=None) – Strongly convex constant if the function g is strongly convex. Allows primal acceleration of the PDHG algorithm.

  • gamma_fconj (positive float, optional, default=None) – Strongly convex constant if the convex conjugate of f is strongly convex. Allows dual acceleration of the PDHG algorithm.

PDHG functions:

  • set_gamma_g (this is due to strong convexity and something we might consider for SPDHG in the future)
  • set_gamma_fconj (this is due to strong convexity and something we might consider for SPDHG in the future)
  • check_convergence
  • set_step_sizes
  • update_step_sizes (this is due to strong convexity and something we might consider for SPDHG in the future)

New SPDHG parameters:

  • f (BlockFunction) – Each must be a convex function with a “simple” proximal method of its conjugate

  • g (Function) – A convex function with a “simple” proximal

  • operator (BlockOperator) – BlockOperator must contain Linear Operators

  • tau positive float, optional, default=None) – Step size parameter for Primal problem

  • sigma (list of positive float, optional, default=None) – List of Step size parameters for Dual problem

  • initial (DataContainer, optional, default=None) – Initial point for the SPDHG algorithm

  • gamma (float) – parameter controlling the trade-off between the primal and dual step sizes

  • sampler (an instance of a cil.optimisation.utilities.Sampler class or another class with the function next(self) implemented outputting an integer from {1,…,len(operator)}.) – Method of selecting the next index for the SPDHG update. If None, a sampler will be created for random sampling with replacement and each index will have probability = 1/len(operator)

New SPDHG functions:

  • set_step_sizes_from_ratio
  • set_step_sizes
  • check_convergence

@MargaretDuff
Copy link
Member Author

See also Gemma's comment #1496 (comment)

@MargaretDuff MargaretDuff mentioned this pull request Apr 15, 2024
9 tasks
@MargaretDuff
Copy link
Member Author

I am struggling with a test which compares PDHG and SPDHG where the number of subsets is one: #1863

@gfardell gfardell added this to the v24.2.0 milestone Jul 15, 2024
@MargaretDuff MargaretDuff requested review from jakobsj and removed request for epapoutsellis and gfardell August 22, 2024 15:12
@MargaretDuff
Copy link
Member Author

As discussed in the software meeting today, we are resurrecting this PR. @paskino @jakobsj - I would be grateful for a brief look

# Gradient descent for the primal variable
# x_tmp = x - tau * zbar
self.x.sapyb(1., self.zbar, -self.tau, out=self.x_tmp)
self.x.sapyb(1., self._zbar, -self._tau, out=self._x_tmp)
Copy link
Contributor

Choose a reason for hiding this comment

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

if self._tau is an array this allocates an array

Copy link
Member Author

Choose a reason for hiding this comment

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

I think fixed so that this doesn't reallocate an array

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Needs reviewing
Status: PRs to review
4 participants