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

Tof #390

Closed
wants to merge 16 commits into from
Closed

Tof #390

wants to merge 16 commits into from

Conversation

ALEXJAZZ008008
Copy link
Member

Partially addresses #315

…. Would need TOF bin selection.

appears to already be in cstir.cpp cSTIR_getAcquisitionsDimensions needs to get number of TOF bins from STIR
@KrisThielemans KrisThielemans added this to In progress in PET TOF support via automation May 24, 2019
@KrisThielemans
Copy link
Member

thanks. Ideally, you modify the travis.yml to use the SuperBuild with appropriate STIR_TAG and STIR_URL. otherwise, please add [ci skip] to commit messages, as we'd get a lot of failures...

PET TOF support automation moved this from In progress to Done Feb 1, 2020
@KrisThielemans
Copy link
Member

why closed?

@ALEXJAZZ008008
Copy link
Member Author

ALEXJAZZ008008 commented Feb 3, 2020

This was accidental, sorry. I wanted to use the name TOF for a more up to date branch and didn't realise it was the one used for this pull request.
I guess you would want the pull request to only be up to commit 72e42db? The commits after are me adding the ability to set the max and min threshold in OSEM and then me setting the required version of STIR and then changing it back (I don't know why this was committed rather than tested and then committed)

@ALEXJAZZ008008 ALEXJAZZ008008 reopened this Feb 3, 2020
PET TOF support automation moved this from Done to In progress Feb 3, 2020
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.

I might have found a problem since my STIR commit UCL/STIR@3f4c94d. Possibly my choice for get_num_sinograms to now include TOF bins wasn't all that great...


dim[0] = sptr_ad->get_num_tangential_poss();
dim[1] = sptr_ad->get_num_views();
dim[2] = sptr_ad->get_num_sinograms();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
dim[2] = sptr_ad->get_num_sinograms();
dim[2] = sptr_ad->get_num_non_tof_sinograms();

@@ -33,26 +33,33 @@ using namespace sirf;
std::string PETAcquisitionData::_storage_scheme;
shared_ptr<PETAcquisitionData> PETAcquisitionData::_template;

float
PETAcquisitionData::norm() const
float PETAcquisitionData::norm() const
Copy link
Member

Choose a reason for hiding this comment

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

revert

Comment on lines +44 to +46

for (seg_iter = seg.begin_all(); seg_iter != seg.end_all();)
{
Copy link
Member

Choose a reason for hiding this comment

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

revert. no code clean-up in a "new feature PR"

Comment on lines +50 to +52

if (s != 0)
{
Copy link
Member

Choose a reason for hiding this comment

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

revert

Comment on lines +54 to +56

for (seg_iter = seg.begin_all(); seg_iter != seg.end_all();)
{
Copy link
Member

Choose a reason for hiding this comment

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

revert

double r = *seg_iter++;
t += r*r;
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

revert

Comment on lines +820 to +825

#if ns >= 16:
#tiles = (4, 4)
#else:
#tiles = None

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why you did this, but I'd rather not do a surprising change in this PR

Comment on lines +831 to +836
#err = show_3D_array(data[0,:,:,:], \
# index = sino[f : t], tile_shape = tiles, \
# label = 'sinogram', \
# xlabel = 'tang.pos', ylabel = 'view', \
# suptitle = title, show = (t == ns))

Copy link
Member

Choose a reason for hiding this comment

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

why commented out?

Comment on lines +2007 to +2021
def set_MAP_model(self, model):
parms.set_char_par\
(self.handle, self.name, 'MAP_model', model)
def set_maximum_relative_change(self, value):
parms.set_float_par\
(self.handle, self.name, 'set_maximum_relative_change', value)
def set_minimum_relative_change(self, value):
parms.set_float_par\
(self.handle, self.name, 'set_minimum_relative_change', value)
def get_objective_function(self):
obj_fun = PoissonLogLikelihoodWithLinearModelForMean()
obj_fun.handle = pystir.cSTIR_parameter\
(self.handle, self.name, 'objective_function')
check_status(obj_fun.handle)
return obj_fun
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 this is here, maybe it needs another merge from master?

Comment on lines +590 to +593
if (boost::iequals(name, "set_maximum_relative_change"))
recon.set_maximum_relative_change(dataFromHandle<double>((void*)hv));
if (boost::iequals(name, "set_minimum_relative_change"))
recon.set_minimum_relative_change(dataFromHandle<double>((void*)hv));
Copy link
Member

Choose a reason for hiding this comment

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

seems a good addition, but shouldn't be a in a TOF PR

ALEXJAZZ008008 added a commit to ALEXJAZZ008008/SIRF that referenced this pull request Feb 8, 2020
PET TOF support automation moved this from In progress to Done Feb 8, 2020
@ALEXJAZZ008008 ALEXJAZZ008008 mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants