STIR
STIR copied to clipboard
UCL
Time Of Flight reconstuction
@NikEfth, did you run the recon_test_pack locally? Travis throws up a lot of problems in it. For instance core dumps in the ROOT tests, but also others. it's always difficult to know with Travis of course.
current Travis failures
The command "./run_test_listmode_recon.sh" exited with 1.
The command "./run_test_zoom_image.sh" exited with 1.
${PYTHON} -m pytest .
collecting 0 items / 1 errors/home/travis/.travis/functions: line 109: 18344 Segmentation fault
The run_test_zoom_image.sh is entirely weird as it doesn't have anything to do with TOF, so maybe ignore that here.
run_test_listmode_recon.sh and the Python test are currently failing
@ALEXJAZZ008008 @NikEfth in my previous commit, I changed ProjData::get_num_sinograms() to include the num-TOF-bins. I did this because we call a Sinogram as 2D array. It also meant that size_all() could be implemented in terms of that, without having to know about TOF. However, this broke create_array_for_proj_data in swig/stir.i as you'll see.
It's easily fixable, but the question is what get_num_sinograms should return. I still think the name implies my change is the best, but feel free to disagree.
This did show that there are currently no Python (or of course MATLAB) tests for the TOF code. Maybe @ALEXJAZZ008008 can contribute some...
oh, maybe the Python test does check it, as I see my comment above that it's failing... Also, listmode recon...
I think get_num_sinograms should return the total number of 2D sinograms. That would include tof_poss and segments. Then individual functions will should be able to return the 3D or TOF positions.
However, now this feels a bit counter-intuitive. Because by default the tof positions are the "outer" layer of our ProjData. Not a big deal, though.
I thought I would just join in to say that I'm still seeing the same problems too.
I'll copy in what I've said elsewhere about the issue for posterity: "With the TOF branch, after it had the master branch merged into it, where previously after forward projecting a volume you would have a sinogram with dimensions (13, 47, 256, 256) you now have a sinogram with dimensions (13, 611, 256, 256) (this is with the same Python code generating the two results). 611 is 13 * 47 so I believe there must be a for loop somewhere that is iterating over both the TOF dimension and the rings (I think) and causing this dimension to be larger than it should be.
I'm surprised this error doesn't throw any of the CTests as I would have expected one of the CTests to check if the size of a sinogram was as expected given the template." When I say CTests here I imagine the issue here would be spotted by a Python test specifically.
I'm still seeing the same issue mentioned above after pulling the TOF branch after Kris' most recent push.
@ALEXJAZZ008008 ok, the sinogram is bigger. How about the information inside? Are the extra positions empty?
@NikEfth The last time I viewed the sinograms visually they appeared to be doughnut shaped with values in an ellipse around a centre of NaN values (which I converted to 0.0 to view)
@ALEXJAZZ008008 it cannot be that the size is still the same. if so, your rebuild didn't work. let us know
@KrisThielemans @NikEfth I did not visualise the sinogram that I forward projected after having pulled the most recent TOF branch. Because it seems that it's not expected that I would still have the same size sinogram after the most recent commits I'll clean my build folder and rebuild from scratch. I can attach a visualisation of the sinogram that I forward project after I have done this.
i cannot get Python to work at all on my Ubuntu 18.04 VM:
ImportError while importing test module '/home/sirfuser/devel/STIR/src/swig/test/python/test_buildblock.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
test_buildblock.py:31: in <module>
from stir import *
../../../../../STIR-install/python/stir.py:17: in <module>
_stir = swig_import_helper()
../../../../../STIR-install/python/stir.py:16: in swig_import_helper
return importlib.import_module('_stir')
/usr/lib/python2.7/importlib/__init__.py:37: in import_module
__import__(name)
E ImportError: /home/sirfuser/devel/STIR-install/python/_stir.so: undefined symbol: _ZN4stir21ForwardProjectorByBin15forward_projectERNS_3BinERKNS_18DiscretisedDensityILi3EfEE
I wonder therefore @ALEXJAZZ008008 if you aren't actually producing anything at all but just looking at previous output.
In any case, the problem is that somewhere in the merge void ForwardProjectorByBin::forward_project(Bin&, const DiscretisedDensity<3,float>&) was dropped from the .cxx. Indeed, this has to be written with the "new style" of PR #362.
By the way, I'm a bit amazed that this function appears in this PR. It isn't TOF at all. And the function seems to be needed for listmode recon anyway (I guess - but didn't check - there it's using ProjMatrixByBin)
@KrisThielemans I don't understand how this could be the case, I have entirely deleted my build and install folder. I've then built and installed using my branch of STIR which when I checked the commit history has the most recent commit from tof_sinos_UCL. I've deleted all of the results directories that I had on my laptop so there are no other .hs files on my PC, then I ran a forward project (using JRMoMo) using the version of STIR (through SIRF) that I had just built and I got the same results as I was describing above. I'll try to attach the .hs and a .png where I've summed across the dimensions of size 13 and 144 of (13, 611, 144, 381)
Regardless, if the function you're describing above is not present in this branch does it mean that it's not possible for this to forward project at all? (in which case I am very confused about what code I'm running exactly)
@KrisThielemans
At your last post, I lost your line of thoughts. Listmode reconstruction uses ProjMatrixElemsForOneBin::forward_project(Bin&, DiscretisedDensity<3,float>&) for the reconstruction.
@ALEXJAZZ008008 That sinogram is full with NaNs ... :/ (at the donut area)
Using the executable, I did a fwd projection of a uniform cylinder to a small sinogram(13, 1, 288, 24, 329) using DSTE as a template.
I cannot spot any NaNs in the sinogram (screenshot across the 13 TOF positions):
P.S. I found 1-2 duplicate lines, probably from the merging but none of them could have causes NaNs. I will look again.
@ALEXJAZZ008008 how is this going? If it is still persistent could you check that, if you let Sirf pull and build its own STIR, if does it for the latest commit of this branch. Maybe it pulls a tag or something ...
@NikEfth You want me to just build vanilla SIRF with the tags set as it would be in the super build if I didn't change them? I assume the version of STIR used by the super build will work otherwise there would be a lot of people who would have noticed this issue there by now.
@ALEXJAZZ008008 No my question is, whether sirf pulls its own copy of STIR and if that copy is indeed up-to-date.
Hi all, I thought I would quickly weight in with results from some testing I did with @ALEXJAZZ008008, hopefully it is useful.
I installed both this branch of the TOF STIR and Alex's modified version of STIR with python support and ran
../install/bin/forward_project test my_atten_image.hv tof.hs
where tof.hs was given to me by @ALEXJAZZ008008,
!INTERFILE := !imaging modality := PT name of data file := /dev/zero originating system := Unknown !version of keys := STIR3.0 !GENERAL DATA := !GENERAL IMAGE DATA := !type of data := PET imagedata byte order := LITTLEENDIAN !PET STUDY (General) := !PET data type := Emission applied corrections := {None} !number format := float !number of bytes per pixel := 4 number of dimensions := 5 matrix axis label [5] := timing positions !matrix size [5] := 13 matrix axis label [4] := segment !matrix size [4] := 1 matrix axis label [3] := view !matrix size [3] := 144 matrix axis label [2] := axial coordinate !matrix size [2] := { 47 } matrix axis label [1] := tangential coordinate !matrix size [1] := 381 %TOF mashing factor := 1 minimum ring difference per segment := { -1 } maximum ring difference per segment := { 1 } Scanner parameters:= Scanner type := GE Discovery 690 Number of rings := 24 Number of detectors per ring := 576 Inner ring diameter (cm) := 81.02 Average depth of interaction (cm) := 0.94 Distance between rings (cm) := 0.654 Default bin size (cm) := 0.21306 View offset (degrees) := -5.021 Maximum number of non-arc-corrected bins := 381 Default number of arc-corrected bins := 331 Energy resolution := 0 Reference energy (in keV) := 511 Number of TOF time bins := 13 Size of timing bin (ps) := 376.5 Timing resolution (ps) := 375 Number of blocks per bucket in transaxial direction := 2 Number of blocks per bucket in axial direction := 4 Number of crystals per block in axial direction := 6 Number of crystals per block in transaxial direction := 9 Number of detector layers := 1 Number of crystals per singles unit in axial direction := 1 Number of crystals per singles unit in transaxial direction := 1 end scanner parameters:= effective central bin size (cm) := 0.226074 number of time frames := 1 start vertical bed position (mm) := 0 start horizontal bed position (mm) := 0 !END OF INTERFILE :=
This successfully runs and produces a 134MB sinogram.
we realised that there are multiple issues here, causing confusing:
- Travis seg-faults on STIR python test
- @kristhielemans can't get the STIR python module to import as above
- @ALEXJAZZ008008 interfaces to STIR via SIRF and has NaN problems when using attenuation in the SIRF acq_model
I've pushed some (innocent) changes that make the first 2 problems disappear on my VM. Let's see what Travis says. It won't fix the SIRF problem though. It's possible that we don't have enough STIR TOF test currently. Or alternatively, the SIRF TOF PR needs some changes.
All green now, with a few extra tests, but probably not enough of them yet.
@ALEXJAZZ008008 , @robbietuk can you provide an update?
Ran list_projdata_info and extract_segments on the data produced by the this TOF branch of STIR. From what @ALEXJAZZ008008 and I could tell, it seemed correct.
I would like to test with @ALEXJAZZ008008 branch of STIR but myriad is down for a few days. If that branch checks out okay, I think that indicates it might be a TOF SIRF problem?
This is now up-to-date with release_4. I plan to do that again after releasing 4.1 (i.e. after merging #601), and then merge with master. Until that happens, the diff is likely horrible, and there are many other conflicts.
@ALEXJAZZ008008 the last commit https://github.com/UCL/STIR/pull/304/commits/0f0cccb30c2abe56f6fb2cb2ff6dbf4ade4cb1d4 could influence you. In particular, it affects https://github.com/SyneRBI/SIRF/pull/543 which was going to be wrong. I guess you must have handled this in your local branch of SIRF
unfortunate to bury this change in a "cleaning" commit as it seems like a bug fix. oh well.
I am not sure how or why that was there. I think is because it made the initialisation of the LM reconstruction simpler. I need to double-check, but I think that there is a confusion with the mashing factors when you run either LM or ProjData.
But I can not remember now.