SyneRBI / SIRF

Main repository for the CCP SynerBI software
http://www.ccpsynerbi.ac.uk
Other
57 stars 29 forks source link

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

Closed KrisThielemans closed 1 week ago

KrisThielemans commented 1 month ago

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 commented 1 month ago

@evgueni-ovtchinnikov wrote in https://github.com/SyneRBI/SIRF/pull/1253#issuecomment-2104760586

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 commented 1 month ago

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 commented 1 month ago

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

evgueni-ovtchinnikov commented 1 month ago

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

gschramm commented 1 month ago

@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 commented 1 month ago

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

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

you can also try https://github.com/UCL/STIR/pull/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 commented 1 month ago

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 commented 1 month ago

Indeed. Listmode subsets currently reproduce those from projection data.

Excellent news!

evgueni-ovtchinnikov commented 1 week ago

should be fixed now that PR #1253 is merged