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

list-mode obj_fun.get_subset_sensitivity(0) returns None #1251

Closed
KrisThielemans opened this issue May 9, 2024 · 9 comments
Closed

list-mode obj_fun.get_subset_sensitivity(0) returns None #1251

KrisThielemans opened this issue May 9, 2024 · 9 comments
Assignees
Labels

Comments

@KrisThielemans
Copy link
Member

Example is at https://github.com/gschramm/SIRF-Exercises/blob/a581dc1e72d3cb1ff156ad88c2db770398a28c17/notebooks/Deep_Learning_listmode_PET/01_SIRF_listmode_recon.py#L176

It works fine for the "sinogram" objective function.

Found by @gschramm.

@KrisThielemans
Copy link
Member Author

@evgueni-ovtchinnikov wrote in #1253 (comment)

which is due to the fact that PoissonLogLikelihoodWithLinearModelForMeanAndListModeDataWithProjMatrixByBin does not override empty ObjectiveFunction.get_subset_sensitivity().

Can you elaborate a bit? get_subset_sensitivity is implemented in PoissonLogLikelihoodWithLinearModelForMean. It also used in PoissonLogLikelihoodWithLinearModelForMeanAndListModeDataWithProjMatrixByBin::actual_compute_subset_gradient_without_penalty. It is of course entirely possible that there is a bug somewhere, but I'd like to understand what you saw.

@evgueni-ovtchinnikov
Copy link
Contributor

In STIR.py we have

class PoissonLogLikelihoodWithLinearModelForMeanAndListModeDataWithProjMatrixByBin(ObjectiveFunction):

From what you write it looks like this class should have been derived from PoissonLogLikelihoodWithLinearModelForMean, should it not?

@KrisThielemans
Copy link
Member Author

KrisThielemans commented May 10, 2024

yes indeed. And it is in STIR. ObjectiveFunction is a SIRF class. Do we have PoissonLogLikelihoodWithLinearModelForMean in SIRF Python?

@evgueni-ovtchinnikov
Copy link
Contributor

yes we do. I have just pushed the corrected STIR.py - let George try it.

@gschramm
Copy link

@evgueni-ovtchinnikov @KrisThielemans was the update pushed to the master branch of SIRF?

Should I re-run the complete Super-Build with DEVEL_BUILD=ON?

I am have some time for tests this late afternoon / tomorrow morning.

@KrisThielemans
Copy link
Member Author

Not yet merged, no. You'll need to set

cmake ... -DSIRF_TAG=origin/acc-hess

you can also try UCL/STIR#1418 by adding

-DSTIR_URL=https://github.com/KrisThielemans/STIR-1.git -DSTIR_TAG=origin/ListModeObjFuncExtras

(although TBH I have never tried switch the URL like this in an existing build, so git status and git log in sources/STIR will be essential).

I'm not 100% sure that SIRF will pick up the LM Hessian, but I think it will. Comments in the respective PRs please.

@gschramm
Copy link

list-mode obj_fun.get_subset_sensitivity(0) returns None is now fixed (now sure which PR fixed it).
Using the SIRF:acc-hess and STIR:master branch, the following test snippet runs without error using a
"realistic" Poisson logL objective function:

for i in range(num_subsets):
    assert np.all(np.isclose(lm_obj_fun.get_subset_sensitivity(i).as_array(), obj_fun.get_subset_sensitivity(i).as_array()))

If the listmode subsets are based on views (which I remember they should be), everything works as expected.

@KrisThielemans
Copy link
Member Author

Indeed. Listmode subsets currently reproduce those from projection data.

Excellent news!

@evgueni-ovtchinnikov
Copy link
Contributor

should be fixed now that PR #1253 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants