Root/scanner rotation changes by robbietuk · Pull Request #1016 · UCL/STIR

robbietuk · 2022-03-30T21:21:04Z

Fix for #938 and extension of #991

enable test_view_offset_root test
Removes the addition of quarter_of_detectors from deterctor position when reading of ROOT files in STIR
Rotate the Gate CylindricalPET scanner by -90 degrees
Remove interfile parameter offset (num of detectors) from all ROOT headers
Update users guide
Reduce number of test
Upload data to Zenodo (see https://zenodo.org/record/6410309)
Automate download of data in CTests

… printing This involves changes to - the coordinates to now use 2D numpy arrays - the print tables now print the position and offset (for loop changes were needed for sizing of every entry etc...), not perfect. - Use more arrays everywhere, instead of lists.

…tationChanges

…elease notes

robbietuk

The removal of offset (num of detectors) and quarter_of_detectors works in the tests now with the rotations in GATE geometry. All our GATE geometries should be updated (thinking STIR-GATE-Connection).

KrisThielemans

Currently using

#if STIR_ROOT_ROTATION_AS_V4

this is non-standard. Replace with using #ifdef as usual. This obviously means that it shouldn't be set by default (I guess you currently set it to 0).

I think the user's guide need some more tweaks but first want to understand Gate macro options better (see comment in the geometry.mac).

You've enabled the test, but it cannot work yet as no data?

KrisThielemans · 2022-03-31T07:23:47Z

Forgot to write my question on the geometry.

You've used rotation of the GATE scanner

STIR/examples/ROOT_files/ROOT_STIR_consistency/Gate_macros/geometry.mac

Line 38 in cccb606

/gate/cylindricalPET/placement/setRotationAngle -90 deg

This is nice, but I do wonder if an alternative solution isn't to modify the placement

STIR/examples/ROOT_files/ROOT_STIR_consistency/Gate_macros/geometry.mac

Line 50 in cccb606

/gate/rsector/placement/setTranslation 41.76 0.0 0.0 cm

I haven't checked of course.

Also removes all mention of commented out ellopsoid parameters

robbietuk · 2022-03-31T23:03:53Z

The source positions are not optimised very well. Running generate_image on each of the STIR par files and summing them results in the following volume

The shown tranaxial slice is at -70mm in z and correspond to generate_image${x}.par files for x in 1 to 6. Another similar transaxial slice (same layout) exists at 70mm in z for generate_image${x}.par files for x in 7 to 12. Also, they are all in the end planes of the scanner (scanner z length L is 157.16mm) so we get no segment information.

The relationship between STIR and GATE z coordinates is stir_z = (L-d)/2 + gate_z, where L is defined above and d is the distance between rings 6.54mm.

I think we change these sources to 8 = 2^3 sources rather than all 12. Two positions per axis, one being the origin, and iterating over each of the combinations, e.g., (in GATE coordinates (x y z) and in mm):

ID	x	y	z	comment
1	0	0	0	center scanner
2	190	0	0	+x
3	0	190	0	+y
4	95	95	0	+x +y
5	0	0	70	+z
6	190	0	70	+x +z
7	0	190	70	+y +z
8	95	95	70	+x +y +z

Slight changes to 7's and 8's x and y values to keep them within the circular FOV (as is currently done). This change not only reduces the data size but also makes the source positions easier to understand. This shouldnt effect TOF either.

edit: Infact we probably could get away with 2 or 4 point source test, e.g. (origin, +x +y +z) or (origin, +x, +x +y, +x +y +z), but life is too short.

EDIT2: THE POINT SOURCE LOCATION HAS SINCE BEEN CHANGED TO THE DATA IN THE TABLE

Correct the z shift between STIR and GATE from 75.21 mm to 75.31 mm.

- Fix issue with `round_sig(x)` function when 0 is input. - Update README with more details of DebugScripts - Comment out `generate_image` from pretest because it is never used. The test was changed to use the `generate_image` class in C++.

…de review) Co-authored-by: Kris Thielemans <[email protected]>

robbietuk · 2022-04-01T00:24:22Z

@francescaleek If you fancy taking a look that would be useful 😄

I optimised almost everything about this test now. The last thing I need to check now is if we can remove the rotation of the cylicricalPET and construct it so the first GATE module is on y axis (x=0).

- release_5.0.htm: moved text on ROOT rotation to "backwards incompatibility" section, with some minor tweaks - STIR User's guide: - expanded the section on ROOT, including translation - added brief info on CMake variables - corrected some text on C++ version

KrisThielemans · 2022-04-03T09:43:51Z

I've updated the documentation a bit more. @robbietuk please check if you're happy with it.

This seems good to merge. The test is enabled though, which means we'd have to download the data, otherwise ctest/GitHub Actions will fail.

zenodo is one option. Adding it to git is another but it'd have to be quite small (preferably less than 2MB). I guess you could see if you tar/gzip -9/bzip it, if size reduces. Then you'd have to uncompress in CMake, see https://stackoverflow.com/a/7052049/3467846 for the opposite problem (creating one).. If you choose that option, make sure it's classified as binary via .gitattributes.

robbietuk · 2022-04-04T12:31:55Z

The data is uploaded to https://zenodo.org/record/6410309 I am working on CMAKE downloads. Do we want to use something like ExternalData for the tests?

robbietuk · 2022-04-05T03:26:50Z

Okay, I am stuck because of my lack of Cmake knowledge. I think the tools are all here but I cant get ExternalData CTests to work... 😢

KrisThielemans · 2022-04-05T11:05:02Z

I don't know about the ExternalData stuff. sounds interesting, but I have no time to look at it.

In SIRF, we use something which is simply, but also a bit of a pain in that it downloads the data even if you don't test. Still, I think putting that into place should be quite easy. We can presumably replace it later if there's smarter stuff around.

have a look at
https://github.com/SyneRBI/SIRF/blob/843eb25760ae8e1e15bc816748d8b80d9c34f7d2/src/Synergistic/tests/CMakeLists.txt#L19

I guess we might have to unzip, but CMake can do that as well https://cmake.org/cmake/help/latest/command/file.html#archive-extract but thi sseems to require CMAke 3.18, so I guess better to download one by one if that's possible.

robbietuk · 2022-04-05T14:52:47Z

In that case, I wont go into usage of the ExternalData tools.

I uploaded each file so we have to download 16 files... It doesnt look pretty but the hash keys are annoying. Good enough for a GHA test

KrisThielemans · 2022-04-05T15:46:18Z

we're not currently installing ROOT in GitHub Actions (nor Appveyor), so I guess this is never tested. Sadly, installing ROOT might not be so trivial as there's no APT package.

if you tell me that this works on your system, we could merge this, and try to get CI going in a separate PR.

robbietuk · 2022-04-05T15:57:33Z

I forgot there were no GHA tests for root, at least it works without ROOT 😄

It works nicely on my machine.

KrisThielemans · 2022-04-05T17:14:10Z

worked on mine!

robbietuk added 6 commits March 30, 2022 10:01

Disable quarter_of_detectors and enable test_view_offset_root

df6ad57

Rotate the Gate geometry

0478e22

Merge branch 'ROOT/removing_quarter_of_detectors' into ROOT/ScannerRo…

cbfd4c6

…tationChanges

[ci skip] offset (num of detectors) is now legacy STIR

efc6c46

[ci skip] STIR GATE Geometry alignment subsubsection in UsersGuide

b5707ac

robbietuk force-pushed the ROOT/ScannerRotationChanges branch from aa6b4b7 to b5707ac Compare March 30, 2022 23:42

robbietuk requested a review from KrisThielemans March 30, 2022 23:42

robbietuk added 2 commits March 30, 2022 17:00

[ci skip] offset (num of detectors) parameter removal documented in r…

853a5ec

…elease notes

[ci skip] remove debugging lines from python script

cccb606

robbietuk commented Mar 31, 2022

View reviewed changes

Comment thread src/include/stir/IO/InputStreamFromROOTFile.inl Outdated

Comment thread src/IO/InputStreamFromROOTFile.cxx

KrisThielemans requested changes Mar 31, 2022

View reviewed changes

Comment thread src/IO/InputStreamFromROOTFile.cxx

Comment thread src/include/stir/IO/InputStreamFromROOTFile.inl Outdated

Comment thread examples/ROOT_files/ROOT_STIR_consistency/Gate_macros/geometry.mac Outdated

Comment thread documentation/release_5.0.htm Outdated

KrisThielemans added this to the v5.0.1 milestone Mar 31, 2022

Refactor source_test#.mac files and replaced deprecated macro entries

c2a15f8

Also removes all mention of commented out ellopsoid parameters

robbietuk and others added 5 commits March 31, 2022 17:09

Reduce the number of point sources to 8 and README changes

172428f

Correct the z shift between STIR and GATE from 75.21 mm to 75.31 mm.

10x activity for point sources with z shift away from origin

c03c9bb

[ci skip] General fixes and README

fe48231

- Fix issue with `round_sig(x)` function when 0 is input. - Update README with more details of DebugScripts - Comment out `generate_image` from pretest because it is never used. The test was changed to use the `generate_image` class in C++.

[ci skip] Change all #if STIR_ROOT_ROTATION_AS_V4 to use #ifdef

94053b8

[ci skip] Release notes and geometry commetry details update (from co…

673bcbf

…de review) Co-authored-by: Kris Thielemans <[email protected]>

KrisThielemans linked an issue Apr 2, 2022 that may be closed by this pull request

ROOT view/detector offset #938

Closed

Add Zenodo test download to Cmake

b17c9b5

KrisThielemans reviewed Apr 5, 2022

View reviewed changes

Comment thread src/recon_test/CMakeLists.txt

[ci skip] Improve cmake log and fix filepath issue

52f81b9

KrisThielemans approved these changes Apr 5, 2022

View reviewed changes

KrisThielemans merged commit b8b1f8a into UCL:master Apr 5, 2022

robbietuk deleted the ROOT/ScannerRotationChanges branch April 5, 2022 17:14

Conversation

robbietuk commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robbietuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KrisThielemans commented Mar 31, 2022

Uh oh!

robbietuk commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robbietuk commented Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrisThielemans commented Apr 3, 2022

Uh oh!

robbietuk commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robbietuk commented Apr 5, 2022

Uh oh!

KrisThielemans commented Apr 5, 2022

Uh oh!

robbietuk commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

KrisThielemans commented Apr 5, 2022

Uh oh!

robbietuk commented Apr 5, 2022

Uh oh!

KrisThielemans commented Apr 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robbietuk commented Mar 30, 2022 •

edited

Loading

robbietuk commented Mar 31, 2022 •

edited

Loading

robbietuk commented Apr 1, 2022 •

edited

Loading

robbietuk commented Apr 4, 2022 •

edited

Loading

robbietuk commented Apr 5, 2022 •

edited

Loading