UCL / STIR

Software for Tomographic Image Reconstruction
http://stir.sourceforge.net/
Other
114 stars 95 forks source link

problem with presmoothing forward and postsmoothing back projectors #606

Open rijobro opened 4 years ago

rijobro commented 4 years ago

These projectors contain the original projector, whilst themselves inheriting from the projector class. As far as I can tell, this will result in duplication of objects. For example, when we call set_up for the forward projector, we do the following:

void
PresmoothingForwardProjectorByBin::
set_up(const shared_ptr<const ProjDataInfo>& proj_data_info_ptr,
       const shared_ptr<const DiscretisedDensity<3,float> >& image_info_ptr)
{
  ForwardProjectorByBin::set_up(proj_data_info_ptr, image_info_ptr);
  original_forward_projector_ptr->set_up(proj_data_info_ptr, image_info_ptr);
}

So now there is a copy of the DiscretisedDensity in both the PresmoothingForwardProjectorByBin class and also in original_forward_projector_ptr.

Before forward projecting, we call set_input. At this point, the data processor will be called on the DiscretisedDensity inside of PresmoothingForwardProjectorByBin. However, when we forward project, we do the following:

void
PresmoothingForwardProjectorByBin::
actual_forward_project(RelatedViewgrams<float>& viewgrams,
                  const int min_axial_pos_num, const int max_axial_pos_num,
                  const int min_tangential_pos_num, const int max_tangential_pos_num)
{
    // No need to do the data processing since it was already done on set_input()
    original_forward_projector_ptr->forward_project(viewgrams,
                                                      min_axial_pos_num, max_axial_pos_num,
                                                      min_tangential_pos_num, max_tangential_pos_num);
}

It looks like, here, the DiscretisedDensity inside of original_forward_projector_ptr gets projected. I don't think the data processor will have been applied to this image, so it will just be a normal forward projection. Further, I can't imagine that set_input will modify the DiscretisedDensity inside of original_forward_projector_ptr, so perhaps the image will never change.

The back projector presumably has a similar problem. Notably, start_accumulating_in_new_image won't start accumulating in a new image.

I'm rather confused on this one, and I hope I've made a mistake somewhere.

If I'm not mistaken, I would vote removing the PreSmoothing and PostSmoothing forward and back projectors altogether.

rijobro commented 4 years ago

Potential problem noted here: https://github.com/UCL/STIR/pull/580.

KrisThielemans commented 4 years ago

Seems to me that PresmoothingForwardProjectorByBin::set_input should apply the data-processor and then use the output for ForwardProjectorByBin::set_input. This class has to mimic whatever we did for the base class.

set_up() stores images but only for geometric info. Their actual image values should never be used. (hence the comments should use Info only).

Obviously, this implies that set_input should first call set_up for the data processor.

rijobro commented 4 years ago

I also think that the objects will be shared across multiple PreSmoothing and PostSmoothing forward and back projectors. What happens if you do:

pre_smoothing_forward_projector_1->set_input(...);
pre_smoothing_forward_projector_2->set_input(...);
pre_smoothing_forward_projector_1->forward(...);

Since they share the same original_forward_projector_ptr, I would expect the forward projection to project the image that was used in the second projectors set_input.

KrisThielemans commented 4 years ago

hmmm. seems you're right. need some thought