-
Notifications
You must be signed in to change notification settings - Fork 44
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
Tv warmstart #1493
Tv warmstart #1493
Conversation
Thanks for your comments @paskino, I have updated the PR and the experiments are still giving similar results. Also for smaller regularisation parameter and more realistic solutions: |
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, however the setter for self.p2
should be changed. This should be discussed with @gfardell
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
@MargaretDuff can you comment about the speed of convergence in terms of wall clock? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking good. Can you remove the comments referencing the lines in the algorithm from the paper. It's useful to include the link to the paper in the docstring but the code may not always line for line reflect the paper. Comments are much less likely to be updated when we do make code changes so could end up confusing things more as they get out of date.
Edo and I chatted, and I've made some changes and scrapped the property and setter and incorporated the changes in to the code. Properties have their place, but I would avoid using them when the call is a much higher cost than the user would expect. I think the user shouldn't be surprised by unintended consequences when from their pov they are just accessing a class member.
Lastly, we need to implement some unit tests and we're good to go!
Co-authored-by: Gemma Fardell <47746591+gfardell@users.noreply.github.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Gemma Fardell <47746591+gfardell@users.noreply.github.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Gemma Fardell <47746591+gfardell@users.noreply.github.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Gemma Fardell <47746591+gfardell@users.noreply.github.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Gemma Fardell <47746591+gfardell@users.noreply.github.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Gemma Fardell <47746591+gfardell@users.noreply.github.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Gemma Fardell <47746591+gfardell@users.noreply.github.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update warmstart
to warm_start
.
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Co-authored-by: Edoardo Pasca <edo.paskino@gmail.com> Signed-off-by: Margaret Duff <43645617+MargaretDuff@users.noreply.github.com>
Done! |
Describe your changes
Added warm start functionality to the CIL total variation function
Describe any testing you have performed
Please add any demo scripts to CIL-Demos/misc/
Testing script added here https://github.com/TomographicImaging/CIL-Demos/blob/main/misc/testing_TV_warmstart.ipynb
Here you can see that:
Using TV with a warm start led to a speed-up in performing implicit PDHG with TV regularisation due to reduced inner iterations required.
PDHG regularised TV with and without warm start converged to a similar reconstructed image.
PDHG regularised by TV with warm start was able to converge to a lower objective function value than without warm start (see e.g. https://link.springer.com/article/10.1007/s10589-022-00410-x)
Changing the number of warm start iterations changes the speed of convergence:
Over the iterations of PDHG, the calculated proximal by TV warm start converges to that of the calculated proximal by the CCPi regulariser
Link relevant issues
supersedes #1201
closes #1200
Checklist when you are ready to request a review
Contribution Notes
Please read and adhere to the developer guide and local patterns and conventions.