Scatter Estimation / Sinogram interpolation in 3D #1156
Conversation
Support for 3D sinogram interpolation, which allows us to run 3D SSS.
There was a problem hiding this comment.
Some initial comments without actual review of the "working" part of the code.
Sadly, there's white-space changes here, which seems a bit arbitrary. I'd prefer to revert these. If there are any, they should be compatible with our clang-format. (we will need to run this through all of STIR, but the TOF PR needs to catch up first)
| extended[y][old_min-1] = extended[y][old_min]; | ||
| extended[y][old_max+1] = extended[y][old_max]; | ||
| } | ||
| std::cout << "n7" << std::endl; |
There was a problem hiding this comment.
these will have to go
| { | ||
| Array<3,float> extended = | ||
| extend_segment_in_views(proj_data_in.get_segment_by_sinogram(0), 2, 2); | ||
| { //TODO: be removed ... |
| @@ -170,7 +174,8 @@ using namespace detail_interpolate_projdata; | |||
|
|
|||
| Succeeded | |||
| interpolate_projdata(ProjData& proj_data_out, | |||
| const ProjData& proj_data_in, const BSpline::BSplineType these_types, | |||
| const shared_ptr<ProjData> proj_data_in, | |||
There was a problem hiding this comment.
why did you change all these types to shared_ptr? It is recommended C++ practice to pass references unless you really need the pointer (e.g. because the function needs to store it after the function exists). Reasons include:
- clearer interface: the user knows you don't need access afterwards
- less dependency on
shared_ptr(I could have aunique_ptrfor instance, which I wouldn't be able to pass).
Of course, STIR isn't quite consistent in this choice, and it is confusing that we mix conventions. But I see no good reason to change the calling conventions here (certainly as proj_data_out is still a reference), but I might be missing something.
If we do need to change it, it should be a const shared_ptr<const ProjData>.
There was a problem hiding this comment.
I was not aware of this. I thought that I was improving the code quality.
OK. I will make the changes.
Where is the |
in the STIR root 😄 |
|
Ok, no worries. Let me know when to pull it or should I wait for the master? |
|
better to wait till it's on master I think. @markus-jehl is going to finish this soon. |
|
Also, let me know if you have questions about the changes I made, @NikEfth. |
This is in good shape. I have tested with data from various scanners.
But more testing is needed and updates in the documentation and such.
@KrisThielemans I am not sure if you would like to change the recon_tests to test this in addition to the existing one.