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

Added BB step size rule to our step size methods #1859

Merged
merged 24 commits into from
Aug 22, 2024

Conversation

MargaretDuff
Copy link
Member

@MargaretDuff MargaretDuff commented Jul 5, 2024

Changes

Adds the BB long and short (from https://en.wikipedia.org/wiki/Barzilai-Borwein_method) step size rule

Adds the stabilisation (adaptive and non-adaptive) from Burdakov, O., Dai, Y.H. and Huang, N., 2019. Stabilized barzilai-borwein method. arXiv preprint arXiv:1907.06409.

Testing you performed

Please add any demo scripts to https://github.com/TomographicImaging/CIL-Demos/tree/main/misc

For a limited angle parallel beam leastsquares +Tikhonov reconstruction using GD the comparison of step size methods is below:
image

For a random matrix Ax=b problem, with a leastsquares objective, reconstruction using GD the comparison of step size methods is below:
image

For these couple of examples "SHORT" is the most stable. Removing the stabilisation has the potential for very fast convergence but is unstable (and not guaranteed to converge!)

Related issues/links

Closes #748

Checklist

  • I have performed a self-review of my code
  • I have added docstrings in line with the guidance in the developer guide
  • I have updated the relevant documentation
  • 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 MargaretDuff self-assigned this Jul 5, 2024
@MargaretDuff MargaretDuff added the enhancement New feature or request label Jul 5, 2024
@MargaretDuff MargaretDuff marked this pull request as ready for review July 8, 2024 10:06
@jakobsj
Copy link
Contributor

jakobsj commented Jul 8, 2024

Thank you, I can take a look. Could you: Clarify what is plotted on the y-axis, maybe better show y as log, or loglog, and I don't see the green one?

@jakobsj
Copy link
Contributor

jakobsj commented Jul 8, 2024

I wasn't aware there so many variants and stabilization methods for BB. We also used BB some years back for a smoothed TV regularization problem and compared with Nesterov acceleration

https://doi.org/10.1007/s10543-011-0359-8

Do you see if these BB methods are related?

@MargaretDuff
Copy link
Member Author

Thank you, I can take a look. Could you: Clarify what is plotted on the y-axis, maybe better show y as log, or loglog, and I don't see the green one?

Thanks @jakobsj, I have updated those plots so hopefully they are a bit clearer

@MargaretDuff
Copy link
Member Author

MargaretDuff commented Jul 8, 2024

I wasn't aware there so many variants and stabilization methods for BB. We also used BB some years back for a smoothed TV regularization problem and compared with Nesterov acceleration

https://doi.org/10.1007/s10543-011-0359-8

Do you see if these BB methods are related?

image
It looks like you do the "LONG" version but then do an additional back tracking procedure.

This could be an option, instead of the "stabilisation" procedure that I use with an option for the user to turn it on or off. I think it was introduced in

Raydan, M.: The Barzilai and Borwein gradient method for the large scale unconstrained minimization problem. SIAM J. Optim. 7, 26–33 (1997)

but I am having trouble accessing it

Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

This is a really great addition and as the experiments demonstrate big speed-up improvements can often be achieved. Nice extensive tests. I added some requests mostly on further elaborating on the documentation and choice of default behaviours.

@jakobsj
Copy link
Contributor

jakobsj commented Jul 9, 2024

I wasn't aware there so many variants and stabilization methods for BB. We also used BB some years back for a smoothed TV regularization problem and compared with Nesterov acceleration
https://doi.org/10.1007/s10543-011-0359-8
Do you see if these BB methods are related?

image It looks like you do the "LONG" version but then do an additional back tracking procedure.

This could be an option, instead of the "stabilisation" procedure that I use with an option for the user to turn it on or off. I think it was introduced in

Raydan, M.: The Barzilai and Borwein gradient method for the large scale unconstrained minimization problem. SIAM J. Optim. 7, 26–33 (1997)

but I am having trouble accessing it

Oh and many thanks for comparing. I think no reason to add the backtracking instead of the stabiliser.

Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

Almost everything nicely addressed, I resolved all comments, and only left one new comment about the None/inf input thing

@jakobsj jakobsj self-requested a review July 11, 2024 18:20
Copy link
Contributor

@jakobsj jakobsj left a comment

Choose a reason for hiding this comment

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

Thanks! "automatic" is really clear and better than the suggested "default". Approving as is - could consider if "auto" instead of "automatic" would be (almost) equally clear/commonly used and briefer.

@gfardell gfardell added this to the v24.2.0 milestone Jul 15, 2024
Copy link
Member

@lauramurgatroyd lauramurgatroyd left a comment

Choose a reason for hiding this comment

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

Sadly found some issues with the rendering of the docs :(

image

@MargaretDuff
Copy link
Member Author

Thanks for reviewing @lauramurgatroyd - looks better
image

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.

Very minor changes left. And happy to discuss if you disagree.

@MargaretDuff MargaretDuff added Merge pending jenkins Once jenkins is happy with can be merged and removed Waiting for review labels Aug 21, 2024
@MargaretDuff
Copy link
Member Author

image
Jenkins is happy

@MargaretDuff MargaretDuff enabled auto-merge (squash) August 22, 2024 09:23
@MargaretDuff MargaretDuff merged commit 1088bc3 into TomographicImaging:master Aug 22, 2024
8 checks passed
@MargaretDuff MargaretDuff deleted the BB-step-size-rule branch August 22, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Merge pending jenkins Once jenkins is happy with can be merged
Projects
No open projects
Status: Needs reviewing
Status: ToDo
Development

Successfully merging this pull request may close these issues.

Implement Barzilai Borwein step size rule
4 participants