Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

InputStreamFromROOTFile changes for virtual crystals #617

Merged
merged 4 commits into from Nov 16, 2020
Merged

InputStreamFromROOTFile changes for virtual crystals #617

merged 4 commits into from Nov 16, 2020

Conversation

KrisThielemans
Copy link
Collaborator

Closing #357

@KrisThielemans
Copy link
Collaborator Author

work in progress

@KrisThielemans KrisThielemans linked an issue Jul 6, 2020 that may be closed by this pull request
enable use of virtual crystals in ROOT processing #357
Closed
@KrisThielemans KrisThielemans force-pushed the ROOT_virtual_crystals branch 5 times, most recently from 4417164 to 962ac45 Compare July 7, 2020 06:01
@KrisThielemans
Copy link
Collaborator Author

This might be working now for the mCT with some limitations/caveats:

  • although you can set the number of virtual crystals via the hroot file, you cannot set it to anything else than what stir::Scanner expects (due to a limitation in Scanner). Therefore, you have to make sure that the hroot file specifies the scanner name correctly (i.e. either "Siemens mCT" or "mCT").
  • although the current run_root_GATE.sh passes, it does only checks on total counts, so I don't know if I broke anything!

Things to do therefore:

  • modify stir:Scanner to allow changing the number of virtual crystals
  • modify run_root_GATE.sh to do some tests on actual coincidence decoding
  • test with mCT GATE output

robbietuk
robbietuk reviewed Jul 7, 2020
View reviewed changes
src/IO/InputStreamFromROOTFile.cxx
@@ -56,6 +66,11 @@ InputStreamFromROOTFile::set_defaults()
low_energy_window = 0.f;
up_energy_window = 1000.f;
read_optional_root_fields=false;
crystal_repeater_x = -1;
crystal_repeater_y = -1;
crystal_repeater_z = -1;
Copy link
Collaborator

@robbietuk robbietuk Jul 7, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really need to be -1? STIR can only handle cases right now where crystal_repeater_z = 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only moved this code from the original location in the derived classes, so no change there. I didn't check if there's actually any error checking or handling of defaults on those values. I agree it seems more logical to let them default to 1 but it doesn't seem high priority

@KrisThielemans
Copy link
Collaborator Author

could test ROOT interpretation in more detail by using for instance

INPUT_ROOT_FILE=test_PET_GATE.root EXCLUDE_SCATTERED=1 EXCLUDE_RANDOM=0 list_lm_events --coincidence 1 --time 0 --num-events-to-list 100 root_header.hroot

A simple thing to do would be to pipe the output of this to a file with a version that is known to work, add the file to the recon_test_pack and in the test, compare the output. We'd have to do the same for the timing events.

@robbietuk robbietuk mentioned this pull request Jul 10, 2020
Open
4 tasks
@KrisThielemans KrisThielemans mentioned this pull request Oct 22, 2020
Closed
@robbietuk
Copy link
Collaborator

I unlisted the same root file from a D690 scanner with lm_to_projdata from using master and this PR branch. I get the same number of events in both sinograms and subtracting one data file from the other gives a file of 0's. It appears that this PR does not affect the current unlisting code regarding unlisting root files in number of events or shape.

@KrisThielemans
move common crystal_repeater_* related functions to InputStreamFromRO…
e989db0 
…OTFile

Also disabled some unused (and incorrect!) InputStreamFromROOTFile* constructors
@KrisThielemans
introduce infrastructure for adding virtual crystals to ROOT
a48dd32 
but it is not yet used
@KrisThielemans
use virtual block info in ROOT
75cad22 
WARNING: backwards incompatible change for scanners with virtual crystals

- add set_num_virtual_*axial_crystals_per_block to Scanner (although it
currently only performs a check if it's consistent with the hardwired
definitions)
- add new keywords in hroot to call set_num_virtual_*axial_crystals_per_block
- modify InputStreamFromROOT::get_num_*axial_crystals_per_block to take
virtual crystals into account.
@KrisThielemans
More documentation on the use of virtual crystals for ROOT
b340311
@KrisThielemans
Copy link
Collaborator Author

I've updated the doc and rebased on master as the history got a bit convoluted (and there was a conflict). should be good to go.

This was referenced Nov 15, 2020
Open
Closed
@KrisThielemans
Copy link
Collaborator Author

2 Travis jobs failed, nothing to do with us (networking error and unknown error). Merging.

@KrisThielemans KrisThielemans merged commit c4c055c into UCL:master Nov 16, 2020
@KrisThielemans KrisThielemans deleted the ROOT_virtual_crystals branch November 16, 2020 16:38
@KrisThielemans KrisThielemans mentioned this pull request Mar 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robbietuk robbietuk robbietuk left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

enable use of virtual crystals in ROOT processing
2 participants
@KrisThielemans @robbietuk