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

Enabling PET reconstruction from list mode data #1103

Merged
merged 52 commits into from
Apr 8, 2024
Merged

Conversation

evgueni-ovtchinnikov
Copy link
Contributor

Fixes #134.

@KrisThielemans
Copy link
Member

looks good to me. Now the objective function...

it should be virtually the same as the one for projection data. Main difference is that it only accepts a projection matrix (not a projector pair).

@evgueni-ovtchinnikov
Copy link
Contributor Author

@NikEfth I increased my VM memory to 16384, and got past set up, but the job was killed (it appears, not long before it would finish):

sirfuser@vagrant:~$ cstir_test6
running test6.cpp...

WARNING: Interfile warning: 'default bin size (cm)' (0.208000) is expected to be 0.200000.


WARNING: Interfile warning: num_axial_blocks_per_bucket (35) is expected to be 7.


WARNING: Interfile warning: axial crystals per singles unit (8) is expected to be 56.


WARNING: Interfile warning: transaxial crystals per singles unit (8) is expected to be 32.


WARNING: Interfile warning: I have used all explicit settings for the scanner
	from the Interfile header, and remaining fields set from the
	UPENN_5rings model.


WARNING: Scanner UPENN_5rings: inconsistent transaxial block info

WARNING: Interfile parsing ended up with the following scanner:
Scanner parameters:= 
Scanner type := UPENN_5rings
Number of rings                          := 280
Number of detectors per ring             := 594
Inner ring diameter (cm)                 := 76.4
Average depth of interaction (cm)        := 0.7
Distance between rings (cm)              := 0.39655
Default bin size (cm)                    := 0.208
View offset (degrees)                    := 0
Maximum number of non-arc-corrected bins := 331
Default number of arc-corrected bins     := 331
Energy resolution := 0.109
Reference energy (in keV) := 511
Number of blocks per bucket in transaxial direction         := 4
Number of blocks per bucket in axial direction              := 35
Number of crystals per block in axial direction             := 8
Number of crystals per block in transaxial direction        := 8
Number of detector layers                                   := 1
Number of crystals per singles unit in axial direction      := 8
Number of crystals per singles unit in transaxial direction := 8
Scanner geometry (BlocksOnCylindrical/Cylindrical/Generic)  := Cylindrical
end scanner parameters:=


image dimensions: 559x200x200
Offset: (-208, -208, -0)
Spacing: (2.08, 2.08, 1.98275)
Size: (200, 200, 559)
Direction matrix: 
1, 0, 0
0, 1, 0
0, 0, -1

== ok

WARNING: WARNING: ProjMatrixByBinUsingRayTracing used for pixel size (in x,y) that is larger than the bin size.
Backprojecting with this matrix might have artefacts at views 0 and 90 degrees.


WARNING: WARNING: ProjMatrixByBinUsingRayTracing used for pixel size (in x,y) that is larger than the bin size.
Backprojecting with this matrix might have artefacts at views 0 and 90 degrees.

tangential LORs: 2
Setting cache path...
Cache path: /home/sirfuser/devel/buildVM/sources/SIRF/data/examples/TBPET/
PoissonLogLikelihoodWithLinearModelForMeanAndListModeData: Skipping input!
Setting scanner template...
Dummy LM file

WARNING: The default max ring difference is set based full Scanner geometry
Setting acquisition model...
Setting max ring diff. ...
Sensitivity images: /home/sirfuser/devel/buildVM/sources/SIRF/data/examples/TBPET/sens_%d.hv
Setting max cache size ...
Max cache: 1500000000
Setting objective function ...
Setting up the reconstruction, please wait ...

WARNING: WARNING: ProjMatrixByBinUsingRayTracing used for pixel size (in x,y) that is larger than the bin size.
Backprojecting with this matrix might have artefacts at views 0 and 90 degrees.


WARNING: WARNING: ProjMatrixByBinUsingRayTracing used for pixel size (in x,y) that is larger than the bin size.
Backprojecting with this matrix might have artefacts at views 0 and 90 degrees.


WARNING: WARNING: ProjMatrixByBinUsingRayTracing used for pixel size (in x,y) that is larger than the bin size.
Backprojecting with this matrix might have artefacts at views 0 and 90 degrees.


WARNING: We skip the check on balanced subsets and presume they are balanced!

WARNING: We skip the check on balanced subsets and presume they are balanced!
Reconstructor set up.
Starting loop with 10 threads
Starting loop with 10 threads
Starting loop with 10 threads
Starting loop with 10 threads
Starting loop with 10 threads
Starting loop with 10 threads
Starting loop with 10 threads
Starting loop with 10 threads
Starting loop with 10 threads
Killed
sirfuser@vagrant:~$ 

Would it be possible to shorten this test run time by quitting early?

@NikEfth
Copy link
Member

NikEfth commented May 23, 2022 via email

@evgueni-ovtchinnikov
Copy link
Contributor Author

@NikEfth Thanks a lot, it works much faster now and does not get killed!

@NikEfth
Copy link
Member

NikEfth commented May 23, 2022

Sure, but the images you get should not make sense

@evgueni-ovtchinnikov
Copy link
Contributor Author

@NikEfth not important at this stage

@KrisThielemans KrisThielemans added this to the v3.3 milestone May 24, 2022
Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

some minor comments which should be safe to implement as far as I can see.

src/xSTIR/cSTIR/include/sirf/STIR/stir_data_containers.h Outdated Show resolved Hide resolved
src/xSTIR/cSTIR/tests/run_test6.cpp Outdated Show resolved Hide resolved
src/xSTIR/cSTIR/tests/run_test6.cpp Show resolved Hide resolved
src/xSTIR/cSTIR/tests/test6.cpp Outdated Show resolved Hide resolved
src/xSTIR/cSTIR/tests/test6.cpp Show resolved Hide resolved
src/xSTIR/pSTIR/STIR.py Outdated Show resolved Hide resolved
src/xSTIR/pSTIR/STIR.py Outdated Show resolved Hide resolved
src/xSTIR/pSTIR/STIR.py Show resolved Hide resolved
Comment on lines 3051 to 3160
if likelihood_type == 'LinearModelForMean':
#if likelihood_type == 'LinearModelForMean':
if likelihood_type is None:
obj_fun = PoissonLogLikelihoodWithLinearModelForMeanAndProjData()
obj_fun.set_acquisition_data(acq_data)
if acq_data is not None:
obj_fun.set_acquisition_data(acq_data)
Copy link
Member

Choose a reason for hiding this comment

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

not sure why we're changing this code here. Do we really need to? What happens if acq_data is None? It'd presumably crash. (Only the listmode case can handle this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KrisThielemans so far as I can see, Nikos' list mode objective function does not need acquisition data, so acq_data must be optional. We need to discuss this with Nikos.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. @NikEfth's listmode function can either use listmode data or the corresponding cache.

However, these lines are for the "old" case with projection data, which do need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will make this function throw in the non-listmode case

Copy link
Member

Choose a reason for hiding this comment

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

presumably the old code did already, but if not, best to put it in, agreed.

I still think we can revert the default to what it as, such that we have minimal code change, but as you prefer.

@KrisThielemans
Copy link
Member

However, then I don't know what to do for the reconstruction.

@evgueni-ovtchinnikov @paskino Introducing an almost empty class on top of DataContainer indeed allows me to use C++ RTTI, so this problem is resolved like this

SPTR_FROM_HANDLE(ContainerBase, sptr_cont, hv);
if (auto sptr_ad = std::dynamic_pointer_cast<STIRAcquisitionData>(sptr_cont)) {
recon.set_input_data(sptr_ad->data_sptr());
}
else if (auto sptr_ld = std::dynamic_pointer_cast<ListmodeData>(sptr_cont)) {
recon.set_input_data(sptr_ld->data_sptr());
}
else
THROW("input_data needs to be either ListmodeData or AcquisitionData");

So I think we can go ahead with this approach.

I haven't checked the Python side yet. We did introduce a ScanData class in Python, but have now no correspondence in C++ anymore. But maybe we don't need it.

- derive ListmodeData from ContainerBase
- add ListmodeData to C wrappers
- add get_info() to STIRImageData and ListmodeData
- expand STIRAcquisitionData::get_info() to add exam-info string
- minimal documentation

This now works fine through Python, but demos still need work.
@KrisThielemans
Copy link
Member

KrisThielemans commented Feb 1, 2024

things to do:

  • Python test
  • remove osem_lm_reconstruction.py example
  • update listmode_reconstruction.py example to use ListmodeData object as opposed to filename
  • change name of objective function

- add ListmodeData::acquisition_data_template()
- let ListmodeToSinograms::set_input() cope with either filename or ListmodeData object
@KrisThielemans
Copy link
Member

listmode_reconstruction.py example now works (i.e. runs through and produces an image. Not 100% sure if it is correct. Could be that the acquisition model isn't passed through completely.

ListmodeToSinograms.set_input() now accepts an ListmodeData object as well.

@KrisThielemans
Copy link
Member

Best to remind all use_gpu lines from the demo, as it isn't supported.

Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

Actually, the whole osem_lm_reconstruction.py needs to be removed. I think everything in it is in listmode_reconstruction.py, which is more complete. Some things on the osem demo are currently not supported anyway.

@@ -23,7 +23,7 @@
-C <cnts>, --counts=<cnts> account for delay between injection and acquisition start by shifting interval to start when counts exceed given threshold.
--visualisations show visualisations
--nifti save output as nifti
--gpu use gpu
--gpu use gpu (actually means: use NiftyPET projectors)
Copy link
Member

Choose a reason for hiding this comment

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

Please keep removed. The STIR listmode recon only supports projmatrixbybin at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean go back to just use gpu?

Copy link
Member

Choose a reason for hiding this comment

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

No. I mean remove all occurrences of use_gpu and the relevant lines to use niftypet, as it won't work anyway. See also main comment

@KrisThielemans
Copy link
Member

I think that later we should move the preparation of randoms, scatter etc to SIRF itself, i.e. taking most of what you wrote for the challenge. However, let's do that in a separate PR, and merge this one asap.

@evgueni-ovtchinnikov
Copy link
Contributor Author

Still have trouble with listmode_reconstruction.py:

sirfuser@vagrant:~/devel/buildVM/sources/SIRF/examples/Python/PET$ python3 listmode_reconstruction.py 
Finding files in /home/sirfuser/devel/install/share/SIRF-3.6/data/examples/PET/mMR

INFO: CListModeDataECAT8_32bit: opening file /home/sirfuser/devel/install/share/SIRF-3.6/data/examples/PET/mMR/list.l

Processing time frame 1
3500000 events stored
Number of prompts stored in this time period : 3612640
Number of delayeds stored in this time period: 0
Last stored event was recorded before time-tick at 50 secs
Total number of counts (either prompts/trues/delayeds) stored: 3612640

This took 2.9s CPU time.
estimate_randoms: trying GEHDF5...
HDF5-DIAG: Error detected in HDF5 (1.10.7) thread 1:
  #000: ../../../src/H5F.c line 292 in H5Fis_hdf5(): unable to determine if file is accessible as HDF5
    major: File accessibility
    minor: Not an HDF5 file
  #001: ../../../src/H5Fint.c line 929 in H5F__is_hdf5(): unable to open file
    major: File accessibility
    minor: Unable to initialize object
  #002: ../../../src/H5FD.c line 741 in H5FD_open(): open failed
    major: Virtual File Layer
    minor: Unable to initialize object
  #003: ../../../src/H5FDsec2.c line 360 in H5FD__sec2_open(): unable to open file: name = 'UNKNOWN', errno = 2, error message = 'No such file or directory', flags = 0, o_flags = 0
    major: File accessibility
    minor: Unable to open file
not a GE HDF5 file. Using ML estimate from delayeds
processed frame 1
Last stored event was recorded after time-tick at 49.999 secs
Total number of prompts/trues/delayed stored: 410099

This took 0.4s CPU time.
Starting iteration 1	KL 247.329
Starting iteration 2	KL 18.2321
Starting iteration 3	KL 0.663053
Starting iteration 4	KL 0.0278851
Starting iteration 5	KL 0.00129212
Starting iteration 6	KL 4.15109e-05
Starting iteration 7	KL 2.26989e-06
Starting iteration 8	KL 3.85613e-08
Starting iteration 9	KL 4.10926e-09
Starting iteration 10	KL 5.79696e-10
CPU time 30.69 secs

WARNING: DiscretisedDensity does not contain any time frames. This might cause an error.
Reading manufacturer PET normalisation file from /home/sirfuser/devel/install/share/SIRF-3.6/data/examples/PET/mMR/norm.n.hdr
trying GEHDF5...
not a GE HDF5 file
applying attenuation (please wait, may take a while)...
estimating scatter (this will take a while!)
Killed

(see also our exchange with Nikos on 23 May 2022 above - back then reducing the size of data helped).

@KrisThielemans
Copy link
Member

I guess this is a problem on your VM. You should probably give it more memory.

It's not clear where this happens. If it's during scatter estimation (as the above implies), reducing number of listmode events is not going to change anything, as the scatter estimate is a (full resolution) projdata.

In any case, the number of events used here is very small.

@KrisThielemans
Copy link
Member

I think that later we should move the preparation of randoms, scatter etc to SIRF itself, i.e. taking most of what you wrote for the challenge. However, let's do that in a separate PR, and merge this one asap.

@evgueni-ovtchinnikov
Copy link
Contributor Author

I guess this is a problem on your VM. You should probably give it more memory.

True. I have increased VM memory from 6GB to 12GB, and listmode_reconstruction.py now runs ok (takes a lot of time though).

Permission to merge?

@KrisThielemans
Copy link
Member

Great. Did you remove the osem_lm_reconstruction.py? Otherwise good to go.

@KrisThielemans
Copy link
Member

Oh. Don't forget to add something to the doc and changes.md

@evgueni-ovtchinnikov evgueni-ovtchinnikov merged commit 2c66faf into master Apr 8, 2024
1 check passed
@evgueni-ovtchinnikov evgueni-ovtchinnikov deleted the lm-recon branch April 8, 2024 11:11
@KrisThielemans KrisThielemans mentioned this pull request Apr 9, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

require STIR 5.1 enable PET listmode reconstruction
4 participants