[SyneRBI/SIRF] Implement adding Hessian diagonal in SIRF (PR #1370)
Evgueni Ovtchinnikov
Changes in this pull request
Testing performed
Related issues
Fixes #1369
Checklist before requesting a review
- I have performed a self-review of my code
- I have added docstrings/doxygen in line with the guidance in the developer guide
- I have implemented unit tests that cover any new or modified functionality
- The code builds and runs on my machine
- CHANGES.md has been updated with any functionality change
Contribution Notes
Please read and adhere to the contribution guidelines.
Please tick the following:
- The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in SIRF (the Work) under the terms and conditions of the Apache-2.0 License.
You can view, comment on, or merge this pull request online at:
https://github.com/SyneRBI/SIRF/pull/1370
Commit Summary
- d95c7bb updated files for release 3.9.0 [ci skip]
- 54505e7 Merge branch 'master' of https://github.com/SyneRBI/SIRF
- dedf83c Merged branch 'master' of https://github.com/SyneRBI/SIRF
- 17867e4 Merge branch 'master' of https://github.com/SyneRBI/SIRF
- 5a1e82f Merge branch 'master' of https://github.com/SyneRBI/SIRF
- 0980bdd started working on adding Hessian diagonal in SIRF
File Changes
(1 file)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- ae578b8 added constructor for xSTIR RDP from Gibbs RDP
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
KrisThielemans left a comment (SyneRBI/SIRF#1370)I think the best strategy is to just use GibbsRelativeDifferencePenalty everywhere. It should be backwards compatible with RelativeDifferencePrior. @ColomboMatte0 is that correct? (we don't need the surrogate stuff here AFAIK)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Matteo Colombo
ColomboMatte0 left a comment (SyneRBI/SIRF#1370)It is backwards compatible with all the methods implemented in RelativedifferencePrior , we do not have SPS stuff for the RDP.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- c9ee3cc trying Kris' suggestion to derive SIRF RDP from STIR Gibbs penalty
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- b67e1b0 replaced STIR RDP with SIRF RDP derived from Gibbs penalty
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- 77754b4 added method compute_Hessian_gradient to SIRF RelativeDifferencePrior
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
KrisThielemans left a comment (SyneRBI/SIRF#1370)@ColomboMatte0 can you suggest a test for this functionality?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
@evgueni-ovtchinnikov pushed 1 commit.
- bcb5a6e removed some commented-out lines [ci skip]
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Evgueni Ovtchinnikov
evgueni-ovtchinnikov left a comment (SyneRBI/SIRF#1370)@ColomboMatte0 could you please suggest a Python test for this functionality. Thank you, E.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Matteo Colombo
ColomboMatte0 left a comment (SyneRBI/SIRF#1370)sorry i lost the previous notification, you mean a test for the hessian diagonal? not really sure what you mean with hessian gradient
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
KrisThielemans left a comment (SyneRBI/SIRF#1370)Yes. For the diagonal (not the gradient). Not sure if we have one in STIR that could be replicated?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Matteo Colombo
ColomboMatte0 left a comment (SyneRBI/SIRF#1370)i remember i tested it numerically using finite differences from the gradient but probably those tests were not added in my PR, i can add it tomorrow, shouldn't be a big deal.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Matteo Colombo
ColomboMatte0 left a comment (SyneRBI/SIRF#1370)$$ H_{ii} \approx \frac{\nabla_i f(x + \epsilon e_i) - \nabla_i f(x - \epsilon e_i)}{2\epsilon} $$
you can test the correctness by computing this, let me know if you want me to implement it somewhere.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Kris Thielemans
KrisThielemans left a comment (SyneRBI/SIRF#1370)Good idea. I suggest to use
$$ H_{ii} \approx \frac{\nabla_i f(x + \epsilon e_i) - \nabla_i f(x)}{\epsilon} $$
a bit easier to implement, and also avoids potential issues with the RDP which is only defined on non-negative images.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.