Skip to content

add SARAH with tests - #2274

Open
epapoutsellis wants to merge 7 commits into
TomographicImaging:masterfrom
epapoutsellis:sarah
Open

add SARAH with tests#2274
epapoutsellis wants to merge 7 commits into
TomographicImaging:masterfrom
epapoutsellis:sarah

Conversation

@epapoutsellis

Copy link
Copy Markdown
Contributor

Description

close #2273

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!

@github-project-automation github-project-automation Bot moved this to Todo in UM 2026 Jan 29, 2026
@casperdcl casperdcl removed this from UM 2026 Jan 29, 2026
@gfardell gfardell added the community-contribution Community-submitted pull requests from contributors outside the core team. label Jan 30, 2026
Comment thread Wrappers/Python/test/test_SARAH.py Outdated
@gfardell gfardell self-assigned this Feb 10, 2026
@gfardell

Copy link
Copy Markdown
Member

Thanks (again!) Vaggelis. A quick look I can see some easy memory saving changes. Maybe once unit tests pass I can make the changes directly before we merge?

Is this referenced in the stochastic paper?

Signed-off-by: Laura Murgatroyd <60604372+lauramurgatroyd@users.noreply.github.com>
Comment thread Wrappers/Python/test/test_SARAH.py Outdated
Comment thread Wrappers/Python/test/test_SARAH.py Outdated
Co-authored-by: Laura Murgatroyd <60604372+lauramurgatroyd@users.noreply.github.com>
Signed-off-by: Laura Murgatroyd <60604372+lauramurgatroyd@users.noreply.github.com>
@MargaretDuff MargaretDuff self-assigned this Aug 27, 2026
@MargaretDuff

MargaretDuff commented Aug 27, 2026

Copy link
Copy Markdown
Member

@epapoutsellis @gfardell @paskino
I made some updates to this PR, mostly moving SARAH to an approximate gradient sum function and not as a stand alone algorithm. I think I have fixed the failing tests and reduced the memory usage as per @lauramurgatroyd and @gfardell comments. There are a few things to discuss:

  • I have gone for a version that is more similar to the paper but doesn't allow you to pass a different approximate gradient method into the SARAH recursion, which I think Vaggelis' argument does though I am not sure if that was the intention
  • I have made a call on data passes that needs sanity checking by someone: if we interpret one data pass as taking information from (on average) all the data then each call to SARAH either adds 1/n data passes or one full data pass, however if we interpret one data pass as the computational cost of computing one full gradient then it would be 2/n or one full data pass. I have gone for the former case, which matches with what we have done for SVRG (in the store_gradients=False it does two gradient evaluations per step but only counts 1/n) and PD3O but I think is different to what this SARAH paper does.
  • I am not sure how to deal with PD3O with a SARAH approximate gradient. PD3O calls the approximate gradient twice per iteration on two different positions, but the SARAH approximate gradient would have accumulated between these two calls. Currently it raises a NotImplementedError.
  • I haven't yet tested on earnest on real data and I am not sure what the update_frequency default should be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Community-submitted pull requests from contributors outside the core team. Waiting for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement SARAH (StochAstic Recursive grAdient algoritHm)

5 participants