SAFIR scanners updates #1563
SAFIR scanners updates #1563
Conversation
KrisThielemans
left a comment
KrisThielemans
left a comment
There was a problem hiding this comment.
sorry for delay!
I believe that the 2 utilities were written by Parisa. I'd therefore prefer that the commit is owner by her. I can fix that, if you're ok with this.
Also, the trim_projdata functionality is already in SSRB I think, so I'd prefer to cut that one out. ok?
|
On Wed, Nov 12, 2025 at 2:17 AM Kris Thielemans ***@***.***> wrote:
***@***.**** requested changes on this pull request.
sorry for delay!
Thanks for your note.
I believe that the 2 utilities were written by Parisa. I'd therefore
prefer that the commit is owner by her. I can fix that, if you're ok with
this.
That's true, I am totally OK with that. I just changed a few things
regarding some pointers to be compatible with new STIR versions, otherwise
they could not be compiled. Thanks.
Also, the trim_projdata functionality is already in SSRB I think, so I'd
prefer to cut that one out. ok?
Since we have used this utility quite a lot in our pipeline and I haven't
changed it, could we keep having this extra utility as well? I think it
doesn't really hurt to have two of them.
… —
Reply to this email directly, view it on GitHub
<#1563 (review)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXA62EKZ7HAB62GXIZNCXY334KDDHAVCNFSM6AAAAABW24VNCSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTINJQHAYDSOBWGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I'm trying to reduce the STIR code-base :-). Could you check if using works the same? If not, I'll keep it. |
|
Also, could you merge |
|
I have tested this before and that's why I say it doesn't work at least for
generic geometry. I get below error:
""
ERROR: The tangential_pos range (-90 to 89) for this projection data is too
large.
Maximum supported range is from -89 to 90
terminate called after throwing an instance of 'std::runtime_error'
what(): The tangential_pos range (-90 to 89) for this projection data is
too large.
Maximum supported range is from -89 to 90
Aborted
""
So I prefer to keep the already-working script by now.
…On Wed, Nov 12, 2025 at 10:15 AM Kris Thielemans ***@***.***> wrote:
*KrisThielemans* left a comment (UCL/STIR#1563)
<#1563 (comment)>
Since we have used this utility quite a lot in our pipeline and I haven't
changed it, could we keep having this extra utility as well? I think it
doesn't really hurt to have two of them.
I'm trying to reduce the STIR code-base :-).
Could you check if using
SSRB -t num_tangential_poss_to_trim output_filename input_projdata_name
works the same? If not, I'll keep it.
—
Reply to this email directly, view it on GitHub
<#1563 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXA62EN5FN5TVXM7IBNTCTL34L3EFAVCNFSM6AAAAABW24VNCSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKMRQHA4DKMZRGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Discussion on that problem is in #1508 and #1507. I cannot see how this is related to the trimming, and am now even more reluctant to include that utility I'm afraid. Sorry. Feel free to keep the utility in your own version of STIR of course. Without the trim utility (and the updates to the doc), I'd be happy to merge. Is it ok to push to your branch to fix ownership? |
added SafirI/SafirII scanners to Scanner class.
SomayehSaghamanesh
force-pushed
the
safir_updates
branch
from
30780c3 to
404647c
Compare
|
I'm confused TBH. I can't spot the difference between SSRB and the trim utility, e.g. Line 73 in 0e667f7 Do you have any idea why SSRB fails but trim_projdata doesn't?
I've tried with some test data to set sorry to be such a pain, but actually these utilities seem to need more updates (TOF etc), and it is going to take more time to do this (and extra work to keep it up-to-date). |
| #include "stir/utilities.h" | ||
| #include "stir/Bin.h" | ||
|
|
||
| #include <fstream> | ||
| #include <iostream> | ||
| #include "stir/ProjDataFromStream.h" | ||
| #include "stir/Viewgram.h" | ||
| #include "stir/IO/read_from_file.h" | ||
| #include "stir/SegmentByView.h" | ||
| #include "stir/ProjDataInterfile.h" | ||
| #include "stir/ProjDataInfo.h" | ||
| #include "stir/LORCoordinates.h" | ||
|
|
||
| #include "stir/GeometryBlocksOnCylindrical.h" | ||
| #include "stir/DetectionPosition.h" | ||
| #include "stir/CartesianCoordinate3D.h" | ||
| #include "stir/DetectorCoordinateMap.h" | ||
| #include <boost/make_shared.hpp> | ||
| #include "stir/CPUTimer.h" | ||
| #include "stir/shared_ptr.h" |
There was a problem hiding this comment.
most of these headers are not used. Certainly not the boost one. Please cut as much as you can.
| #include "stir/shared_ptr.h" | ||
|
|
||
|
|
||
| #ifndef STIR_NO_NAMESPACES |
There was a problem hiding this comment.
remove this #ifdef. we're on C++-17 now
| shared_ptr<ProjDataInfo> in_pdi_ptr(in_pd_ptr->get_proj_data_info_sptr()->clone()); | ||
| shared_ptr<ProjDataInfo> out_pdi_ptr(template_pd_ptr->get_proj_data_info_sptr()->clone()); | ||
| ProjDataInterfile out_proj_data(template_pd_ptr->get_exam_info_sptr(), out_pdi_ptr, output_filename+".hs"); | ||
| write_basic_interfile_PDFS_header(output_filename+".hs", out_proj_data); |
There was a problem hiding this comment.
I don't think we need this
|
|
||
| timer0.start(); | ||
|
|
||
| assert(in_pdi_ptr->get_min_segment_num()==-1*in_pdi_ptr->get_max_segment_num()); |
There was a problem hiding this comment.
remove. this isn't relevant here.
| // keep sinograms out of the loop to avoid reallocations | ||
| // initialise to something because there's no default constructor | ||
| Viewgram<float> viewgram_blk = out_proj_data.get_empty_viewgram(out_proj_data.get_min_view_num(),seg); | ||
| Viewgram<float> viewgram_cyl = in_pd_ptr->get_empty_viewgram(in_pd_ptr->get_min_view_num(),seg); | ||
|
|
||
| for(int view=in_pdi_ptr->get_min_view_num(); view<=in_pdi_ptr->get_max_view_num();++view) | ||
| { | ||
| viewgram_blk = out_proj_data.get_empty_viewgram(view,seg); | ||
| viewgram_cyl = in_pd_ptr->get_viewgram(view,seg); | ||
|
|
||
| for(int ax=in_pdi_ptr->get_min_axial_pos_num(seg); ax<=in_pdi_ptr->get_max_axial_pos_num(seg);++ax) | ||
| { | ||
| for(int tang=in_pdi_ptr->get_min_tangential_pos_num(); tang<=in_pdi_ptr->get_max_tangential_pos_num(); ++tang) | ||
| { | ||
| viewgram_blk[ax][tang] = viewgram_cyl[ax][tang]; | ||
| } | ||
| } | ||
| if (!(out_proj_data.set_viewgram(viewgram_blk)== Succeeded::yes)) | ||
| warning("Error set_segment for projdata_symm %d\n", seg); | ||
| } |
There was a problem hiding this comment.
we can shorten this a bit, and make it ready for TOF along the lines of
SegmentIndices seg_indices(seg); // TODO add TOF here later
auto seg_blk = out_proj_data.get_empty_segment_by_view(seg_indices);
const auto seg_cyl= out_proj_data.get_segment_by_view(seg_indices);
std::copy(seg_cyl.begin_all(), seg_cyl.end_all(), seg_blk.begin_all());which means we need to #include "stir/SegmentIndices.h" and <algorithm>.
Obviously, the naming of the variables isn't quite correct, as we don't know what the type is, but fine.
| \ingroup utilities | ||
|
|
||
| \brief This program takes a projection data from one type and converts it to another type. | ||
| \author Parisa Khateri |
There was a problem hiding this comment.
as I'm asking you to clean this up, please add yourself. We'll forget about fixing the ownership for this file, as it's going to be very short.
| std::string output_filename=argv[1]; | ||
| shared_ptr<ProjData> in_pd_ptr = ProjData::read_from_file(argv[2]); | ||
| shared_ptr<ProjData> template_pd_ptr = ProjData::read_from_file(argv[3]); | ||
|
|
There was a problem hiding this comment.
ideally add a check that get_num_tof_poss()==0 and if not, call error
|
Hi. sorry for my late response, I was away.
Regarding SSRB not working for trimming projections, as you can see from
the message, it is related to the same problem as tangential position range
inconsistency. Since I am trimming projections from tang180, it can not
handle it with this inconsistency.
Probably if I trim it from another tang, it will work, but this won't be
useful for us.
…On Wed, Nov 12, 2025 at 1:12 PM Kris Thielemans ***@***.***> wrote:
*KrisThielemans* left a comment (UCL/STIR#1563)
<#1563 (comment)>
I'm confused TBH. I can't spot the difference between SSRB and the trim
utility, e.g.
https://github.com/UCL/STIR/blob/0e667f713967f45f76cbb906805e3058e9e09f20/src/buildblock/SSRB.cxx#L73
Do you have any idea why SSRB fails but trim_projdata doesn't?
I've tried with some test data to set num_tangential_poss in the .hs
equal to num_detectors_per_ring, and SSRB is fine with it.
sorry to be such a pain, but actually these utilities seem to need more
updates (TOF etc), and it is going to take more time to do this (and extra
work to keep it up-to-date).
—
Reply to this email directly, view it on GitHub
<#1563 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXA62EP7MRRGFHQO2LSA4WD34MP25AVCNFSM6AAAAABW24VNCSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKMRRGYZDSMRVHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I will go through the changes and let you know. |
2 participants
Changes in this pull request
Added two models of SAFIR scanners (generic geometry) to stir's scanners list.
Added two utilities to convert and trim projections.
Testing performed
STIR-6 build tests were performed successfully without any error. Tests for SAFIR I/II data conversion and image reconstructions were performed successfully.
Related issues
issue #1501
Checklist before requesting a review
documentation/release_XXX.mdhas been updated with any functionality change (if applicable)