[FIX] Constructing STIR Image from Acq Data with nx, ny should use Acq data

[ashgillman]

Previously, if nx, ny was specified, then the resulting VoxelsOnCartGrid didn't actually use the constructor with AcqData

[KrisThielemans]

tiny change.

@evgueni-ovtchinnikov, after @ashgillman fixes this, can you squash-merge?

[ashgillman]

can you please also add a doxygen comment?

Sure, but I'm not sure where this goes? There seems to by doxygen only for the class not the methods/constructors?

[ashgillman]

The build fails seem to be perhaps some network error? Or misconfiguration?

[KrisThielemans]

Might be good to have defaultsny=nx,nx=-1`

[KrisThielemans]

Travis failures are due to SyneRBI/SIRF-SuperBuild#119, which needs @casperdcl but he's travelling.

[evgueni-ovtchinnikov]

sorry cannot see where you set new voxel sizes

[KrisThielemans]

@rijobro, is this PR still relevant? @ashgillman can you merge master into here to resolve conflicts. we should then finish it off.

The proposed change is much shorter/clearer than the original. However...

sorry cannot see where you set new voxel sizes

There is a difference between these 2 versions I believe. I think @evgueni-ovtchinnikov preserved FOV size, while @ashgillman preserves voxel-size. Both make sense, but are in a way not sufficient as you want to be able to choose any number of voxels and voxel size. STIR uses zoom for this, but it is in my opinion too confusing (it requires you to know the default_bin_size which is an out-dated concept). I don't think people would want to have different x,y voxel-size, so possibly we should have a constructor (symbolically)

ImageData(acq_data, num_of_voxels_xy=[-1-1], max_diameter_in_mm=-1)

where the default max_diameter is obtained from the acq_data, e.g. schematically

D=max(proj_data.get_s_in_mm(acq_data.get_max_tang_pos_num()),
            - proj_data.get_s_in_mm(acq_data.get_min_tang_pos_num()))*2

Then it'll be a bit of fun to find the correct STIR zoom. Maybe

xy_vox_size= D/std::max(nx,ny);
zoom=proj_data.get_proj_data_info_sptr()->get_scanner_sptr()->get_default_bin_size()/xy_vox_size;

Do we have a c++ data-type to store the num_voxels_xy?

[KrisThielemans]

hi, can we progress this one please?

[ashgillman]

I've merged and updated the doc. Re: Evguenni's Q - STIR's VoxelsOnCartesianGrid likes to determine the voxel sizes, as Kris mentions. I'm not sure on the best way forward, depending on how much control one would want over this.

[KrisThielemans]

I think people will want to change the FOV diameter. Example: if you're doing brain, there's no point reconstructing lots of zero voxels of course. @evgueni-ovtchinnikov 's way to scale voxel-size seems more intuitive than STIR's. So, I still think that my suggestion is ok. On the C++ side I guess we'd need

ImageData(const PETAcquisitionData& acq_data, 
                   const std::pair<int>num_voxels_xy=std::make_pair(-1,-1), 
                   const float max_diameter_in_mm=-1)

Relevant code in STIR as around here. It'd obviously be better to avoid having this stuff in SIRF, but that'd need a similar constructor in STIR, which needs a bit more thought.

[KrisThielemans]

@ashgillman any chance we can finalise this before v2.0?

[ashgillman]

I didn't quite understand the first time - I thought you meant make changes into STIR to suit. But if I understand you mean just work around STIR convention? I had a quick look a few weeks ago but have lost track, I'll have a look again this week.

[KrisThielemans]

As STIR now supports different x,y,z zooms,
https://github.com/UCL/STIR/blob/e5edc12bd44d6243dad94bd5eed93717815e1ee8/src/include/stir/VoxelsOnCartesianGrid.h#L137
it's better to use a pair for max_diameter_in_mm as well