Spdhg with stochastic sampler #1644

MargaretDuff · 2024-01-10T11:19:44Z

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

With 80 subsets:

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

Quick docstring Signed-off-by: Margaret Duff <[email protected]>

…ate prob in spdhg

Signed-off-by: Margaret Duff <[email protected]>

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

paskino · 2024-10-01T09:54:51Z

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

+    sampler: optional, 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)
+    prob_weights: optional, list of floats of length num_indices that sum to 1. Defaults to [1/len(operator)]*len(operator)
+            Consider that the sampler is called a large number of times this argument holds the expected number of times each index would be called,  normalised to 1. Note that this should not be passed if the provided sampler has it as an attribute. 


Can we explain "Note that this should not be passed if the provided sampler has it as an attribute."?

Maybe:
Note: if the sampler has a prob_weight attribute it will take precedence on this parameter.

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

paskino · 2024-10-01T10:15:49Z

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

+            else:
+                return False


do you need this?

I changed it to return a value error instead. Basically we can't check it for non-scalar values of tau

paskino · 2024-10-01T10:18:42Z

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

+        self._zbar.sapyb(self._tau,  self.x, -1., out=self._x_tmp )
+        self._x_tmp*=-1


if self._tau is a number I don't see the reason of this change as it forces you to have an additional loop

Yes, but i think your point was that if self.tau was an array then changing these two lines means that you don't allocate memory doing -self.tau

paskino · 2024-10-01T11:13:06Z

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

+        self._sampler = sampler
+
+        self._prob_weights = getattr(self._sampler, 'prob_weights', None)
+        if prob_weights is not None:
+            if self._prob_weights is None:
+                self._prob_weights = prob_weights
+            else:
+                raise ValueError(
+                    ' You passed a `prob_weights` argument and a sampler with attribute `prob_weights`, please remove the `prob_weights` argument.')
+
+        self._deprecated_kwargs(deprecated_kwargs)
+
+        if self._prob_weights is None:
+            self._prob_weights = [1/self._ndual_subsets]*self._ndual_subsets
+
+        if self._sampler is None:
+            self._sampler = Sampler.random_with_replacement(
+                len(operator), prob=self._prob_weights)
+
+        self._norms = operator.get_norms_as_list()


Let's simplify this part.

Hi Edo, I had a go and tried to explain the reasoning in the comments.

gfardell

Mostly docstring clarification. I think it's very close.

gfardell · 2024-10-16T10:55:31Z

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

    tau : positive float, optional, default=None
-        Step size parameter for Primal problem
+        Step size parameter for primal problem. If `None` see note.


Remove default=None I think it's fine to just say optional (we use that elsewhere in CIL).

The description should say it'll be computed.

How about:

tau : positive float, optional Step size parameter for the primal problem. If `None` will be computed by algorithm, see note for details.`

The same comment applies to all the arguments in the docstring.

gfardell · 2024-10-16T11:13:58Z

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

+    sampler: optional, 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)`


This probably needs to be clearer.

sampler: cil.optimisation.utilities.Sampler, optional A `Sampler` controllingthe selection of the next index for the SPDHG update. If `None`, a sampler will be created for uniform random sampling with replacement. See notes. Note ----- The `sampler` can be an instance of the `cil.optimisation.utilities.Sampler` class or a custom class with the `__next__(self)` method implemented, which outputs an integer index from {1, ..., len(operator)}. Note ----- "Random sampling with replacement" will select the next index with equal probability from `1 - len(operator)`.

gfardell · 2024-10-16T11:15:14Z

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

-        parameter controlling the trade-off between the primal and dual step sizes
+    gamma : float, optional
+            Parameter controlling the trade-off between the primal and dual step sizes
+    sampler: optional, 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)}. 


Is it really from {1,...,len(operator)}?

We should use zero indexing everywhere so this might be a typo or a bigger issue.

Good spot, we do index from 0 so a typo and not a bigger issue

gfardell · 2024-10-16T11:19:06Z

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

+    prob_weights: optional, list of floats of length `num_indices` that sum to 1. Defaults to `[1/len(operator)]*len(operator)`
+            Consider that the sampler is called a large number of times this argument holds the expected number of times each index would be called,  normalised to 1. Note that this should not be passed if the provided sampler has it as an attribute: if the sampler has a `prob_weight` attribute it will take precedence on this parameter. 


the input type needs to be concise. list of floats, optional with the description expanding.

Beyond that, it's not clear why we need this and where it's used. ISn't this what sampler now controls? So either it's a docstring or implementation problem. Should it be moved to kwargs?

There was a design decision made that the sampler doesn't have to have prob_weights as in other stochastic algorithms they are not essential, just for reporting and plotting. However, this algorithm requires them to set sigma and tau. As the sampler might not have that argument, instead it can be passed seperately to the algorithm. But maybe we chat about it being a kwarg...

gfardell · 2024-10-16T11:33:30Z

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

+
+
+        # Set up sampler and prob weights from deprecated "prob" argument
+        sampler = self._deprecated_set_prob(deprecated_kwargs, prob_weights, sampler) 


this isn't very clear. I can see if I pass sampler=None and prob a sampler is created. Could this be handled directly by the if statement in ln 167 that creates a sampler?

Otherwise the naming implies it's just about setting prob, which I think would be sufficient. Then if Sampler is None it gets created as planned with self._prob_weights

I can see you're trying to split the deprecated set up from the main code, but it's confusing that it's creating the sampler in 2 places.

I think sorted

gfardell · 2024-10-16T11:35:04Z

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py

+        self._deprecated_set_norms(deprecated_kwargs) 
+        self._norms = operator.get_norms_as_list()
+        #Check for other kwargs
+        self._deprecated_else(deprecated_kwargs)


I'm not sure this is worth having as a function, just a check on unused kwargs would maybe suffice if it's needed

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

First attempt at sampling class

8389932

Changed how probabilities and samplers interact in SPDHG

7331c73

Update sampling.py

2bce666

Quick docstring Signed-off-by: Margaret Duff <[email protected]>

Changed to factory method style and added in permuatations

ea759c5

Debugging and fixing random generator in show epochs

d1909a3

Testing SPDHG

98b0694

Changed the show epochs

05b67cb

Meeting with Vaggelis, Jakob, Gemma and Edo

001350b

Set up for installation

890dec0

Added staggered and custom order and started with writing documentation

25806fc

Work on documentation

75abbfe

Commenting and examples in the class

ebdf329

Debugging sampler

ba35fb8

Changes after dev meeting

beac6fa

Checking probabilities in init

1202e53

initial testing

079935b

Sped up PDHG and SPDHG testing

43e3dc4

Removed timing statements

004ab2f

Got rid of epochs - still need to fix the shuffle

7b857e0

Fixed random without replacement shuffle=False

1f7d546

Changes after meeting 12-09-2023. Remove epochs in sampler and deprec…

6993a95

…ate prob in spdhg

Sampler unit tests added

bafc748

Some checks for setting step sizes

d62aa2b

Started looking at unit tests and debugging SPDHG setters and init

c81b71c

Notes after discussions with gemma

b28f2f1

Changes after discussion with gemma

4a87f48

Updated tests

b35222f

Just a commenting change

6e552af

Initial changes and tests- currently failing tests

6575af6

Sorted tests and checks on the set_norms function

6b463bc

Merge branch 'master' into spdhg_sampler

b67d977

Signed-off-by: Margaret Duff <[email protected]>

casperdcl force-pushed the master branch 3 times, most recently from a910556 to f8b513b Compare September 2, 2024 13:11

Changes to sapyb call to save memory

2b11156

MargaretDuff requested review from paskino and gfardell September 2, 2024 15:26

MargaretDuff added 2 commits September 2, 2024 16:27

Merge branch 'master' into spdhg_sampler

1be0029

Merge branch 'master' into spdhg_sampler

2edd0e8

lauramurgatroyd removed this from the v24.2.0 milestone Sep 24, 2024

paskino reviewed Oct 1, 2024

View reviewed changes

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py Outdated Show resolved Hide resolved

paskino reviewed Oct 1, 2024

View reviewed changes

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py Outdated Show resolved Hide resolved

paskino reviewed Oct 1, 2024

View reviewed changes

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py Outdated Show resolved Hide resolved

paskino reviewed Oct 1, 2024

View reviewed changes

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py Outdated Show resolved Hide resolved

paskino reviewed Oct 1, 2024

View reviewed changes

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py Outdated Show resolved Hide resolved

paskino reviewed Oct 1, 2024

View reviewed changes

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py Outdated Show resolved Hide resolved

paskino reviewed Oct 1, 2024

View reviewed changes

Wrappers/Python/cil/optimisation/algorithms/SPDHG.py Outdated Show resolved Hide resolved

paskino reviewed Oct 1, 2024

View reviewed changes

MargaretDuff and others added 5 commits October 2, 2024 10:00

Edo's comments

9da703a

Merge branch 'master' into spdhg_sampler

0b42f5c

Updates following discussion with Edo

0addbdb

Merge branch 'master' into spdhg_sampler

76780c8

Changed sampler init

e8641d0

paskino approved these changes Oct 15, 2024

View reviewed changes

Fix to failing test

b1f0dbb

gfardell requested changes Oct 16, 2024

View reviewed changes

Updates from Gemma's comments

36c0bf9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spdhg with stochastic sampler #1644

Spdhg with stochastic sampler #1644

MargaretDuff commented Jan 10, 2024 •

edited

Loading

paskino Oct 1, 2024 •

edited

Loading

paskino Oct 1, 2024

MargaretDuff Oct 2, 2024

paskino Oct 1, 2024

MargaretDuff Oct 2, 2024

paskino Oct 1, 2024

MargaretDuff Oct 2, 2024

gfardell left a comment

gfardell Oct 16, 2024

gfardell Oct 16, 2024

MargaretDuff Oct 16, 2024

gfardell Oct 16, 2024

MargaretDuff Oct 16, 2024

gfardell Oct 16, 2024

MargaretDuff Oct 16, 2024

gfardell Oct 16, 2024

MargaretDuff Oct 16, 2024

gfardell Oct 16, 2024

MargaretDuff Oct 16, 2024

		self._zbar.sapyb(self._tau, self.x, -1., out=self._x_tmp )
		self._x_tmp*=-1

		sampler: optional, 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)`

		prob_weights: optional, list of floats of length `num_indices` that sum to 1. Defaults to `[1/len(operator)]*len(operator)`
		Consider that the sampler is called a large number of times this argument holds the expected number of times each index would be called, normalised to 1. Note that this should not be passed if the provided sampler has it as an attribute: if the sampler has a `prob_weight` attribute it will take precedence on this parameter.



		# Set up sampler and prob weights from deprecated "prob" argument
		sampler = self._deprecated_set_prob(deprecated_kwargs, prob_weights, sampler)

Spdhg with stochastic sampler #1644

Are you sure you want to change the base?

Spdhg with stochastic sampler #1644

Conversation

MargaretDuff commented Jan 10, 2024 • edited Loading

Describe your changes

Describe any testing you have performed

Link relevant issues

Checklist when you are ready to request a review

Contribution Notes

paskino Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfardell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MargaretDuff commented Jan 10, 2024 •

edited

Loading

paskino Oct 1, 2024 •

edited

Loading