Scatter Estimation / Sinogram interpolation in 3D - #1156
Conversation
KrisThielemans
left a comment
KrisThielemans
left a comment
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 ... |
| 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. |
|
Is this PR still useful or completely superseded? |
|
I don't think it is completely obsolete. Certainly, there's nothing on It'd be great to have a 3D upsampler of course, but I haven't check the current approach nor do I know what other things are going on in Groningen, but I suppose you do. |
|
Not sure what you mean about other things in Groningen |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| UnusedCode | 1 medium |
| BestPractice | 1 medium |
🟢 Metrics 98 complexity · 5 duplication
Metric Results Complexity 98 Duplication 5
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@Aparna1403J @KrisThielemans This is moving forward quite well I think. Next step the michelogram interpolation and this will be finished. |
Support for 3D sinogram interpolation, which allows us to run 3D SSS.
* Guard if enough axial positions exist for the type of interpolation that is asked * First step of interpolation is finished, michelogram to follow.
As tests are starting piling up I want to paste here a comparison of the output of |
* One bug fix for the oblique sinograms, corrected
3 participants
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.