ProxSkip algorithm by epapoutsellis · Pull Request #2267 · TomographicImaging/CIL

epapoutsellis · 2026-01-28T11:42:57Z

Description

Example Usage

Contribution Notes

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

❤️ Thanks for your contribution!

gfardell · 2026-02-24T11:58:31Z

Thanks Vaggelis, this looks like a great addition. I'll add it to the work plan and make sure it gets prioritised for review.

epapoutsellis · 2026-09-22T14:06:54Z

+class ProxSkip(Algorithm):
+
+
+    r"""Proximal Skip  (ProxSkip) algorithm, see "ProxSkip: Yes! Local Gradient Steps Provably Lead to Communication Acceleration! Finally!†"


Need to add ref paper @epapoutsellis

gfardell

Hi @epapoutsellis, I have added some comments. Please let me know when you are ready for me to review, or if you need any support in addressing them. I haven't looked closely at the unit tests yet.

gfardell · 2026-09-30T15:47:16Z

+    def _update_previous_solution(self):
+        """ Swaps the references to current and previous solution based on the :func:`~Algorithm.update_previous_solution` of the base class :class:`Algorithm`.
+        """
+        tmp = self.x_new
+        self.x = self.x_new
+        self.x = tmp


This isn't switching the references as intended. Instead it's orphaning the buffer that was at self.x.

gfardell · 2026-09-30T15:51:32Z

+            self.g.proximal(self.xhat_new - (self.step_size/self.prob)*self.ht, self.step_size/self.prob, out=self.x_new)
+            self.ht.sapyb(1., (self.x_new - self.xhat_new), (self.prob/self.step_size), out=self.ht)            


These are both allocating new ImageData's for the intermediate steps. This should use a preallocated buffer.

Added new buffer.

gfardell · 2026-09-30T15:53:16Z

+        # convention: use proximal in the first iteration
+        if self.iteration==0:
+            theta = True
+        self.thetas.append(theta)


Is there a reason to log these?

It is useful for the user to see where proximal is computed. I created a new function that can be called after running the algo.

gfardell · 2026-09-30T15:56:07Z

+        if self.prob<=0:
+            raise ValueError("Need a positive probability")
+        if self.prob==1:
+            raise warn("If p=1, ProxSkip is equivalent to ISTA/PGD. Please use ISTA/PGD to avoid computing updates of the control variate that is not used.")


This should be a warnings.warn() not raise warn. It also should have a unit test check.

gfardell · 2026-09-30T15:56:44Z

+
+        logging.info("{} setting up".format(self.__class__.__name__, ))        
+
+        ## TODO better to use different initials for x and h.        


ToDo in code.

gfardell · 2026-09-30T16:00:37Z

+        self.xhat_new -= self.ht
+        self.x.sapyb(1., self.xhat_new, -self.step_size, out=self.xhat_new)
+
+        theta = self.rng.random() < self.prob 


If prob=1, theata is true and proximal step is called. But the docstring says prob is the probability to skip the proximal step.

gfardell · 2026-09-30T16:03:05Z

@@ -0,0 +1,112 @@
+from cil.optimisation.algorithms import Algorithm


No file header. It should be the licence and you added as a contibutor. As a note we also should update your instituation in the notice.txt

gfardell · 2026-09-30T16:05:47Z

+
+        self.configured = True
+
+        logging.info("{} configured".format(self.__class__.__name__, ))


The module log should be created at the top of the file with
log = logging.getLogger(__name__) then log.info()

gfardell · 2026-09-30T16:06:25Z

+            A convex function with a "simple" proximal.
+        prob : positive :obj:`float`
+             Probability to skip the proximal step. If :code:`prob=1`, proximal step is used in every iteration.             
+        step_size : positive :obj:`float`


Could this accept a StepSizeRule as Ista and Fista do?

Similarly, can it take a preconditioner like ISTA or FISTA??

The reason that I do not inherit for ISTA is that at the moment there is no theory for adaptive step sizes or precond when skipping. Only heuristic choices that are not necessarily faster (in time) compared to a ISTA. So I think is dangerous for the users to allow this option.

gfardell · 2026-09-30T16:07:04Z

+        logging.info("{} setting up".format(self.__class__.__name__, ))        
+
+        ## TODO better to use different initials for x and h.        
+        self.initial = initial


This adds a reference to initial that is never used.

MargaretDuff

A few quick comments

MargaretDuff · 2026-10-06T11:06:42Z

+
+
+    r"""Proximal Skip  (ProxSkip) algorithm, see "ProxSkip: Yes! Local Gradient Steps Provably Lead to Communication Acceleration! Finally!†"
+


Could we also add a few lines about what optimisation objective the algorithm solves and why you would want to use it?

MargaretDuff · 2026-10-06T11:09:45Z

+from warnings import warn
+
+
+class ProxSkip(Algorithm):


Is there a reason it is not a subclass of ISTA/PGD?

The reason that I do not inherit for ISTA is that at the moment there is no theory for adaptive step sizes or precond when skipping. Only heuristic choices that are not necessarily faster (in time) compared to a ISTA. So I think is dangerous for the users to allow this option.

MargaretDuff · 2026-10-06T11:10:22Z

+            A convex function with a "simple" proximal.
+        prob : positive :obj:`float`
+             Probability to skip the proximal step. If :code:`prob=1`, proximal step is used in every iteration.             
+        step_size : positive :obj:`float`


Similarly, can it take a preconditioner like ISTA or FISTA??

epapoutsellis · 2026-10-26T10:12:43Z

Thank you @gfardell @MargaretDuff for your reviews . I've made a few changes to address your comments.

Signed-off-by: Vaggelis Papoutsellis <22398586+epapoutsellis@users.noreply.github.com>

gfardell · 2026-10-28T12:18:51Z

@epapoutsellis thanks! I will take another look in the next week.

epapoutsellis added 7 commits January 28, 2026 11:42

add ProxSKip algo

5d93289

initial tests, need more, CVXpy not working on mac

4dbaea7

ISTA vs ProxSkip, more tests needed, test signature

0df1ff6

more tests, problem with random seed

b23f405

fix coin flip test

aa270f4

fix input function

259b324

local prob with cvxpy, uncomment

d95f7bd

casperdcl added this to UM 2026 Jan 29, 2026

github-project-automation Bot moved this to Todo in UM 2026 Jan 29, 2026

epapoutsellis requested review from MargaretDuff and paskino January 29, 2026 11:13

casperdcl removed this from UM 2026 Jan 29, 2026

gfardell added the community-contribution Community-submitted pull requests from contributors outside the core team. label Jan 30, 2026

gfardell assigned paskino and gfardell Feb 10, 2026

lauramurgatroyd added this to CIL work Sep 9, 2026

github-project-automation Bot moved this to Todo in CIL work Sep 9, 2026

epapoutsellis commented Sep 22, 2026

View reviewed changes

lauramurgatroyd requested review from gfardell and removed request for paskino September 22, 2026 14:08

gfardell requested changes Sep 30, 2026

View reviewed changes

MargaretDuff reviewed Oct 6, 2026

View reviewed changes

lauramurgatroyd added this to the v26.1 milestone Oct 8, 2026

changes after review

7330128

Merge branch 'master' into proxskip

61822ee

Signed-off-by: Vaggelis Papoutsellis <22398586+epapoutsellis@users.noreply.github.com>

lauramurgatroyd moved this from Blocked to Priority review in CIL work Oct 27, 2026

		class ProxSkip(Algorithm):


		r"""Proximal Skip (ProxSkip) algorithm, see "ProxSkip: Yes! Local Gradient Steps Provably Lead to Communication Acceleration! Finally!†"

		self.g.proximal(self.xhat_new - (self.step_size/self.prob)*self.ht, self.step_size/self.prob, out=self.x_new)
		self.ht.sapyb(1., (self.x_new - self.xhat_new), (self.prob/self.step_size), out=self.ht)


		logging.info("{} setting up".format(self.__class__.__name__, ))

		## TODO better to use different initials for x and h.

		@@ -0,0 +1,112 @@
		from cil.optimisation.algorithms import Algorithm


		self.configured = True

		logging.info("{} configured".format(self.__class__.__name__, ))

Conversation

epapoutsellis commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Example Usage

Contribution Notes

Uh oh!

gfardell commented Feb 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfardell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MargaretDuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epapoutsellis commented Oct 26, 2026

Uh oh!

gfardell commented Oct 28, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

epapoutsellis commented Jan 28, 2026 •

edited

Loading