Virtual xtal interface. #776

NikEfth · 2020-12-22T02:53:02Z

Interface for the virtual xtals.

Update CListModeDataROOT.cxx, Scanner.h, and 4 more files...

NikEfth · 2020-12-22T04:16:10Z

This PR addresses the issues raised in #750

Update CListModeDataROOT.cxx, Scanner.inl, and 2 more files...

Update Scanner.cxx

…p blocks.

KrisThielemans

Most of this is fine. However, you're changing the interpretation of the "standard" function like get_num_axials_crystal_per_block to mean the "non-virtual" ones, while it was "all of them". Although I agree that the naming is confusing, I'd rather avoid changing this as it probably has much wider consequences. For instance, what does get_num_rings mean? It was "number of all rings, non-virtual+virtual". If we'd change that, lots of things will have to be adapted.

src/IO/InterfileHeader.cxx

KrisThielemans · 2020-12-22T10:54:36Z

src/include/stir/Scanner.inl

@@ -160,7 +160,7 @@ Scanner::get_num_axial_blocks() const
 int
 Scanner::get_num_transaxial_blocks() const
 {
-  return num_detectors_per_ring/num_transaxial_crystals_per_block;
+  return num_detectors_per_ring/(num_transaxial_crystals_per_block+num_virtual_transaxial_crystals_per_block);


cannot change this.

I still disagree on this one. Currently num_transaxial_crystals_per_block includes both. We shouldn't be adding.

Then in come down to what we include in the total number of detectors in the template.
If the total number of detectors include the virtual or not.
My initial notion is that they should not. BUT if you don't you have problems with the get_bin() function (wrong inversions).

So, until we restructure the class, the total number of rings and detectors per ring should include the virtual.

KrisThielemans · 2020-12-22T10:55:20Z

src/listmode_buildblock/CListModeDataROOT.cxx

        ok = false;
    }

-    if (scanner_sptr->get_num_detectors_per_ring() != root_file_sptr->get_num_dets_per_ring())
+    if ((scanner_sptr->get_num_detectors_per_ring() - scanner_sptr->get_num_virtual_transaxial_crystals()) != root_file_sptr->get_num_dets_per_ring())


cannot change this

I guess we could change it after all, if we say that the root_file_sptr->get_num_dets_per_ring() returns the number of physical detectors per ring, which is probably logical.

Needs serious documentation though

But I would still find it very confusing if get_num_dets_per_ring() means different things for Scanner and the ROOT stuff

KrisThielemans · 2020-12-22T10:58:48Z

src/buildblock/Scanner.cxx

+//    default:
+//      return 0;
+//    }
+    return num_virtual_axial_crystals_per_block * get_num_axial_blocks();


this is probably going to be incorrect. The mCT doesn't use a virtual crystal for the last block of course (apparently Tahereh did for her scanner).

Would have to be derived from num_rings somehow. Do we need this function?

Yes I know.
I don't know what to propose here. Say that you add one that the end. I don't think that it actually changes much. Where could this create a problem? Normalisation? Multiple bed positions? I guess the axial middle of the scanner will include that ring and that might be a problem.

What we could do is check with the num_rings + gaps - 1,

@tniknejad Is this something you could change?

I'm very confused. you introduced this function. We have get_num_virtual_axial_crystals_per_block which is well-defined. why do we need this?

src/buildblock/Scanner.cxx

KrisThielemans · 2020-12-22T10:59:58Z

src/buildblock/Scanner.cxx

@@ -746,12 +790,12 @@ check_consistency() const
      {
 	const int dets_per_ring =
 	  get_num_transaxial_blocks() *
-	  get_num_transaxial_crystals_per_block();
+      (get_num_transaxial_crystals_per_block() + get_num_virtual_transaxial_crystals_per_block());


shouldn't be changed

KrisThielemans · 2020-12-22T11:00:20Z

src/buildblock/Scanner.cxx

@@ -782,7 +826,7 @@ check_consistency() const
      {
 	const int dets_axial =
 	  get_num_axial_blocks() *
-	  get_num_axial_crystals_per_block();
+      (get_num_axial_crystals_per_block() + get_num_virtual_axial_crystals_per_block());


shouldn't be changed I believe

KrisThielemans · 2020-12-22T11:00:36Z

src/buildblock/Scanner.cxx

@@ -817,12 +861,12 @@ check_consistency() const
 	      this->get_name().c_str());
    else
      {
-	if ( get_num_detectors_per_ring() % get_num_transaxial_crystals_per_singles_unit() != 0)
+    if ( get_num_detectors_per_ring() % (get_num_transaxial_crystals_per_singles_unit() + get_num_virtual_transaxial_crystals_per_block()) != 0)


shouldn't be changed

KrisThielemans · 2020-12-22T11:00:44Z

src/buildblock/Scanner.cxx

 	  { 
 	    warning("Scanner %s: inconsistent transaxial singles unit info:\n"
-		    "\tnum_detectors_per_ring %d should be a multiple of num_transaxial_crystals_per_singles_unit %d",
+            "\tnum_detectors_per_ring %d should be a multiple of num_transaxial_crystals_per_singles_unit + virtual %d",


shouldn't be changed

But this is what you have in mCT.
This is an important comment: If you don't include the virtual crystals in the total number of detectors per ring the unlisting fails. The sinogram is broken. I could send an image to show you.

It has something to do with the inversion in the tangestial positions when the tang_pos is larger than the number of views. Which currently is calculated from the total number of detectors per ring.

If you don't include the virtual crystals in the total number of detectors per ring the unlisting fails. The sinogram is broken.

of course. Siemens does all calculations/sinograms in terms of "total number". Nowhere in their files there are any "virtual" ones (even not in the norm, presumably because they do gap-filling)

KrisThielemans · 2020-12-22T11:00:49Z

src/buildblock/Scanner.cxx

 		    this->get_name().c_str(),
-		    get_num_detectors_per_ring(), get_num_transaxial_crystals_per_singles_unit()); 
+            get_num_detectors_per_ring(), (get_num_transaxial_crystals_per_singles_unit() + get_num_virtual_transaxial_crystals_per_block()));


shouldn't be changed

NikEfth · 2020-12-22T17:04:21Z

The definition of the scanner overall is very circular and confusing and probably we should re-write from scratch.

However, I took the lead from the mCT example were the number of the detectors per ring includes the virtual crystals (13+1)*48. Respectively the num_rings should include the virtual rings.

The changes, I made allow us to design a scanner if a 2-fold way, 1 gap per block (in mCT every 13 crystals) or 1 gap every several blocks. The later is in better agreement with most scanners I would know, where the block separation between blocks is small of e.g. half crystal, but there is 1 or 2 crystal between the rotated (bigger) blocks (r-sectors in GATE).

For example, my scanner has 18 sectors with 4 transaxial blocks with 8 transaxial crystals.
Now both number of detectors per ring 18*(4*8+1) and 18*4*(8+1) can pass the consistency test.
And soon the CListModeDataROOT will be able to know were to push the gap.

I think that bein able to reposition the gaps is quite important.

NikEfth · 2020-12-22T17:07:56Z

I would like to add that the modification does not interfere with the singles units, which can still be independent.

KrisThielemans · 2020-12-22T18:39:50Z

The definition of the scanner overall is very circular and confusing and probably we should re-write from scratch

yeah. at some point... You cleaned at least the set_params stuff up in #304. Let's not attempt a major rewrite until #304 and #577 are merged.

The changes, I made allow us to design a scanner if a 2-fold way, 1 gap per block (in mCT every 13 crystals) or 1 gap every several blocks. The later is in better agreement with most scanners I would know, where the block separation between blocks is small of e.g. half crystal, but there is 1 or 2 crystal between the rotated (bigger) blocks (r-sectors in GATE).

I certainly agree that many scanners would have bigger gaps between "buckets" than "blocks". However, I hadn't realised that you wanted to fix that with this PR. The name of the PR and previous conversation led me to think you only wanted to be able to set the number of virtual crystals (which is what most of this PR is about).

In fact, I cannot see how you could fix this without having additional members num_axial_virtual_crystals_per_bucket etc (or proably better num_additional_axial_virtual_crystals_per_bucket as it'd add up to the one between blocks).

To me adding that facility seems to need a fair amount of work, but maybe I'm wrong.

NikEfth · 2020-12-22T18:46:51Z

Well, I am not really fixing it, for that we would need a num_virtual_crystal_per_bucket() function, but just letting the consistency test pass and the make the CListModeROOT do the right thing.
The key logic is that based on the number of detector per ring you set, we can figure out how many virtual crystals there are. Is the divisor of block number or block*crystal.

I understand that some tests succeed but there might be further implications down the line.

Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>

NikEfth · 2020-12-23T02:45:39Z

hmm, after all, I introduced the gaps in buckets.
~~My main question is whether you prefer the num_of_detectors_per_ring in the template to be the physical detectors only, or both gaps and detectors~~
~~I think that the second is easier. Otherwise, we have to go everywhere and replace the get_numXX() functions with get_num_logicalXX()~~
~~While if the total number in the template includes the virtual crystals we have to introduce functions get_num_phisicalXX() in fewer cases~~

Actually, I think that in the template we should set the number of physical detectors and the then get functions to do the calculations. However, this means that we cannot have set functions. There have to become explicitly set_physical, or very very very well documented. What do you think?

int
Scanner::get_num_rings() const
{  return get_num_physical_rings() + (get_num_virtual_axial_crystals_per_bucket() - 1) *  get_num_axial_buckets() +
            (get_num_virtual_axial_crystals_per_block()-1) * get_num_axial_blocks();}
int
Scanner::get_num_detectors_per_ring() const
{
  return num_detectors_per_ring + (get_num_virtual_transaxial_crystals_per_bucket() - 1) * get_num_transaxial_buckets() +
          (get_num_virtual_transaxial_crystals_per_block()-1) * get_num_transaxial_blocks();}
int
Scanner::get_num_physical_rings() const
{  return num_rings; 
}
int
Scanner::get_num_physical_detectors_per_ring() const
{
  return num_detectors_per_ring;
}

…buckets.

KrisThielemans · 2021-02-04T11:19:15Z

I think that in the template we should set the number of physical detectors and the then get functions to do the calculations. However, this means that we cannot have set functions. There have to become explicitly set_physical, or very very very well documented. What do you think?

While reading/setting things in terms of physical detectors would make a lot of sense, changing the meaning of get_num_rings() to suddenly be get_num_physical_rings() would break quite a lot of stuff. Given my pace of accepting PRs, I'd strongly recommend not attempting that... Of course, making it clearer in the documentation what is what would help.

Of course, having members get_num_physical_rings(), get_num_non_physical_rings() (with the sum get_num_rings() ) is an excellent idea. Providing an alias for get_num_rings() that makes it clear what it means is of course fine as well. We could then consider deprecating the old names.

KrisThielemans

I've given this another look and added some comments. I didn't check consistency of everything. I also didn't think about the "singles units" stuff.

I guess I'm fine with changing the internal meaning of some of the private variables like Scanner::num_rings if you prefer, but I don't think we should change the meaning of get_num_rings() etc (which you don't of course, so that's good). It could be confusing for a developer of Scanner, but it can be addressed by documentation.

I really think though that we cannot change the meaning of num_rings etc in the Interfile header. It's too late for that.

KrisThielemans · 2021-02-04T11:27:24Z

src/include/stir/Scanner.h

  inline int get_num_rings() const;
-  //! get the number of detectors per ring
+  //! get the number of detectors per ring. This function returns the total
+  //! number of detectors + gasp in the ring.


Suggested change

//! number of detectors + gasp in the ring.

//! number of (physical and virtual) detectors in the ring.

KrisThielemans · 2021-02-04T11:27:42Z

src/include/stir/Scanner.h

@@ -188,9 +192,10 @@ class Scanner
  //! \name Functions returning geometrical info
  //@{

-  //! get number of rings
+  //! get number of rings. This function returns the total number of rings + gaps


Suggested change

//! get number of rings. This function returns the total number of rings + gaps

//! get number of rings. This function returns the total number of (physical and virtual) rings

KrisThielemans · 2021-02-04T11:28:24Z

src/include/stir/Scanner.h

@@ -302,9 +311,11 @@ class Scanner
  // zlong, 08-04-2004, add set_methods
  //! set scanner type
  inline void set_type(const Type & new_type);
-  //! set number of rings
+  //! set number of rings. This function will set the total number of rings
+  //! reseting the virtual crystals to the axial direction to 0.


Suggested change

//! reseting the virtual crystals to the axial direction to 0.

//! resetting the virtual crystals in the axial direction to 0.

KrisThielemans · 2021-02-04T11:29:06Z

src/include/stir/Scanner.h

+  //! set the namber of detectors per ring. This function will set the total number
+  //! of detectors per ring, reseting the number of virtual crystals to 0.


Suggested change

//! set the namber of detectors per ring. This function will set the total number

//! of detectors per ring, reseting the number of virtual crystals to 0.

//! set the number of detectors per ring. This function will set the total number

//! of detectors per ring, resetting the number of virtual crystals (in transaxial direction) to 0.

KrisThielemans · 2021-02-04T11:32:35Z

src/include/stir/Scanner.inl

 }

 int
 Scanner::get_num_transaxial_blocks() const
 {
-  return num_detectors_per_ring/num_transaxial_crystals_per_block;
+  return num_transaxial_crystals_per_block == 0 ? num_detectors_per_ring :


num_transaxial_crystals_per_block should never be zero. It doesn't make sense in STIR. You might have only 1 block, but no crystals in a block means no crystals

KrisThielemans · 2021-02-04T11:32:43Z

src/include/stir/Scanner.inl

 }

 int
 Scanner::get_num_axial_buckets() const
 {
-  return get_num_axial_blocks()/num_axial_blocks_per_bucket;
+  return num_axial_blocks_per_bucket == 0 ? get_num_axial_blocks() :


same as above

KrisThielemans · 2021-02-04T11:35:07Z

src/listmode_buildblock/CListModeDataROOT.cxx

        ok = false;
    }

-    if (scanner_sptr->get_num_detectors_per_ring() != root_file_sptr->get_num_dets_per_ring())
+    if ((scanner_sptr->get_num_detectors_per_ring() - scanner_sptr->get_num_virtual_transaxial_crystals()) != root_file_sptr->get_num_dets_per_ring())


But I would still find it very confusing if get_num_dets_per_ring() means different things for Scanner and the ROOT stuff

KrisThielemans · 2021-02-04T11:35:40Z

src/IO/InterfileHeader.cxx

@@ -600,6 +600,12 @@ InterfilePDFSHeader::InterfilePDFSHeader()
  num_transaxial_crystals_per_block = 0;
  add_key("number of crystals_per_block in transaxial direction",
 	  &num_transaxial_crystals_per_block);
+  num_virtual_axial_crystals_per_block = 0;


seems we'd need some bucket stuff here as well

KrisThielemans · 2021-10-01T07:27:01Z

@NikEfth I cannot recall where we were with this, but aside from the comments above, it probably needs a revisit now that #833 has been merged.

Virtual xtal interface.

f8fb42a

Update CListModeDataROOT.cxx, Scanner.h, and 4 more files...

NikEfth added the enhancement label Dec 22, 2020

NikEfth added 3 commits December 22, 2020 00:55

Test_Scanner pass and able to run lm_to_projdata

4c65b2f

Update CListModeDataROOT.cxx, Scanner.inl, and 2 more files...

Forgot to commit the hunk for the rings.

ac7d7c0

Update Scanner.cxx

Update Scanner.cxx to be able to apply to gap on every block, or grou…

a72bfdf

…p blocks.

KrisThielemans requested changes Dec 22, 2020

View reviewed changes

NikEfth and others added 3 commits December 22, 2020 13:58

Update src/buildblock/Scanner.cxx

5bea447

Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>

Update src/IO/InterfileHeader.cxx

ca8438a

Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>

Initial commit for additional gaps on every bucket.

c46df95

NikEfth added 2 commits December 23, 2020 19:17

Proposal for Scanner template with virtual crystals after blocks and …

61fc997

…buckets.

Merge branch 'work_on_virtual_xtals' of git@github.com:NikEfth/STIR.git

80b9d73

KrisThielemans requested changes Feb 4, 2021

View reviewed changes

KrisThielemans mentioned this pull request Feb 10, 2021

virtual crystal for ml_norm 3d #713

Closed

KrisThielemans added the in-progress label Oct 1, 2021

KrisThielemans mentioned this pull request Apr 29, 2022

Scanner::check_consistency problem with virtual crystals #629

Open

KrisThielemans mentioned this pull request Feb 8, 2024

Axial bucket projection issue #1375

Merged

robbietuk mentioned this pull request Feb 15, 2024

Expansion to Buckets on Cylindrical geometries #1385

Closed

	//! number of detectors + gasp in the ring.
	//! number of (physical and virtual) detectors in the ring.

	//! get number of rings. This function returns the total number of rings + gaps
	//! get number of rings. This function returns the total number of (physical and virtual) rings

	//! reseting the virtual crystals to the axial direction to 0.
	//! resetting the virtual crystals in the axial direction to 0.

		//! set the namber of detectors per ring. This function will set the total number
		//! of detectors per ring, reseting the number of virtual crystals to 0.

Virtual xtal interface. #776

Are you sure you want to change the base?

Virtual xtal interface. #776

Conversation

NikEfth commented Dec 22, 2020

NikEfth commented Dec 22, 2020

KrisThielemans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikEfth commented Dec 22, 2020 • edited

NikEfth commented Dec 22, 2020

KrisThielemans commented Dec 22, 2020

NikEfth commented Dec 22, 2020

NikEfth commented Dec 23, 2020 • edited

KrisThielemans commented Feb 4, 2021

KrisThielemans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented Oct 1, 2021

NikEfth commented Dec 22, 2020 •

edited

NikEfth commented Dec 23, 2020 •

edited