Allow even number of ToF bins by danieldeidda · Pull Request #1334 · UCL/STIR

danieldeidda · 2024-01-15T16:20:26Z

…aps found

…et_k and swap s sym

KrisThielemans · 2024-01-16T07:39:05Z

@danieldeidda test_DataSymmetriesForBins_PET_CartesianGrid failed. Possibly we made a mistake in the merge, or there is something else.

danieldeidda · 2024-01-16T14:56:18Z

something is happening with the segmentIndices::_timing_pos whcich occasionally is different. Need to go deeper in debugging though

danieldeidda · 2024-01-17T11:23:10Z

src/include/stir/recon_buildblock/DataSymmetriesForBins_PET_CartesianGrid.inl

      else // segment_num = 0 
      {
-        if ( do_symmetry_swap_s && s < 0) return new SymmetryOperation_PET_CartesianGrid_swap_xmx_ymy(view180, axial_pos_shift, z_shift);
+        if ( do_symmetry_swap_s && s < 0) return new SymmetryOperation_PET_CartesianGrid_swap_xmx_ymy(view180, axial_pos_shift, z_shift, proj_data_info_ptr->get_scanner_ptr()->get_max_num_timing_poss());


I guess the problem is happening in one of these modifications

Actually not sure anymore because the test is giving errors when the 2 bins are actually the same. also it happens for a non-tof test

can you paste some sample errors, for both TOF and non-TOF?

WARNING: FactoryRegistry:: overwriting previous value of key in registry.
key: None
Tests for DataSymmetriesForBins_PET_CartesianGrid
Testing span=1

INFO: Detected view-mashing factor 24 from the number of views (8) and the number of detectors per ring (384).
Adjusting the azimuthal angle offset accordingly (an extra offset of 10.7813 degrees)
Tests with usual image size

INFO: Determined voxel size by dividing default_bin_size (3.12932) by zoom
Testing with all symmetries

WARNING: Disabling symmetries as image is rotated due to phi offset of the scanner.
Error. Comparing symmetry bin configuration

Current bin: segment = -5, axial pos 0, view = 0, tangential_pos_num = -6, timing position index = 0
Symm bin: segment = -5, axial pos 0, view = 0, tangential_pos_num = -6, timing position index = 0
Error. Comparing symmetry bin configuration

Current bin: segment = -5, axial pos 0, view = 0, tangential_pos_num = -3, timing position index = 0
Symm bin: segment = -5, axial pos 0, view = 0, tangential_pos_num = -3, timing position index = 0
Error. Comparing symmetry bin configuration

Current bin: segment = -5, axial pos 1, view = 0, tangential_pos_num = -6, timing position index = 0
Symm bin: segment = -5, axial pos 1, view = 0, tangential_pos_num = -6, timing position index = 0
Error. Comparing symmetry bin configuration

Current bin: segment = -5, axial pos 1, view = 0, tangential_pos_num = -3, timing position index = 0
Symm bin: segment = -5, axial pos 1, view = 0, tangential_pos_num = -3, timing position index = 0
Error. Comparing symmetry bin configuration

Current bin: segment = -5, axial pos 2, view = 0, tangential_pos_num = -6, timing position index = 0
Symm bin: segment = -5, axial pos 2, view = 0, tangential_pos_num = -6, timing position index = 0
Error. Comparing symmetry bin configuration

Current bin: segment = -5, axial pos 2, view = 0, tangential_pos_num = -3, timing position index = 0
Symm bin: segment = -5, axial pos 2, view = 0, tangential_pos_num = -3, timing position index = 0
Error. Comparing symmetry bin configuration

Current bin: segment = -5, axial pos 3, view = 0, tangential_pos_num = -6, timing position index = 0
Symm bin: segment = -5, axial pos 3, view = 0, tangential_pos_num = -6, timing position index = 0
Error. Comparing symmetry bin configuration

Current bin: segment = -5, axial pos 3, view = 0, tangential_pos_num = -3, timing position index = 0
Symm bin: segment = -5, axial pos 3, view = 0, tangential_pos_num = -3, timing position index = 0
Error. Comparing symmetry bin configuration

Current bin: segment = -5, axial pos 4, view = 0, tangential_pos_num = -6, timing position index = 0
Symm bin: segment = -5, axial pos 4, view = 0, tangential_pos_num = -6, timing position index = 0
Error. Comparing symmetry bin configuration

Current bin: segment = -5, axial pos 4, view = 0, tangential_pos_num = -3, timing position index = 0
Symm bin: segment = -5, axial pos 4, view = 0, tangential_pos_num = -3, timing position index = 0

KrisThielemans · 2024-01-22T22:52:39Z

test failure in OSMAPOSL_parallelproj is weird as non-TOF, but occurs in all Release builds (and isn't run in the Debug jobs), so not a one-off.

test_proj_data_info failures:

printing of bins is also flawed (replace as in other file) so we can't see what's going on
Debug runs have assert error https://github.com/UCL/STIR/actions/runs/7614648272/job/20737408998?pr=1334#step:10:921
worrying is Error. malloc(): corrupted top size https://github.com/UCL/STIR/actions/runs/7614648272/job/20737408026?pr=1334#step:10:1110

Other debug asserts:

src/include/stir/ProjDataInfoCylindricalNoArcCorr.inl

src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndProjData.cxx

KrisThielemans · 2024-01-22T22:59:40Z

src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndProjData.cxx

+    this->max_num_central_timing_poss_to_process =
+      this->proj_data_sptr->get_max_tof_pos_num();


I think is wrong, it probably needs a divide by 2 or so

mh not sure than the shift is different then zero if we want to use them all

in fact, we don't support this yet, but having wrong code is a bad idea. Add a comment and throw an error if the argument isn't what we expect?

However, it seems hard to know what the argument should be. For 2 TOF bins, the max number of central should be 1. For 3 TOF bins, it should also be 1.5... (1 could work, but how would one say then we only want to do the central one). Looks like the name is still wrong.

Or remove...

src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndProjData.cxx

src/test/test_proj_data_info.cxx

KrisThielemans · 2024-01-27T09:21:00Z

I've merged master to resolve conflict. Sadly, the MacOS build now fails because it requires C++-14 (due to upgraded boost version). https://github.com/UCL/STIR/actions/runs/7677624387/job/20926592594?pr=1334#step:9:1146

/usr/local/include/boost/math/tools/config.hpp:23:6: warning: #warning "The minimum language standard to use Boost.Math will be C++14 starting in July 2023 (Boost 1.82 release)" [-Wcpp]
   23 | #    warning "The minimum language standard to use Boost.Math will be C++14 starting in July 2023 (Boost 1.82 release)"

KrisThielemans · 2024-02-01T20:19:41Z

@danieldeidda I'd really like to release STIR 6.0.0 soon. We can easily postpone this to 6.1. Let me know.

danieldeidda · 2024-02-02T11:28:14Z

Hi I have been looking at it but haven't been able to find the source of the error. Let's postpone to 6.1 then

src/include/stir/recon_buildblock/DataSymmetriesForBins_PET_CartesianGrid.inl

danieldeidda

this seems to have fixed the problem. Let's see how the test goes

src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndProjData.cxx

KrisThielemans · 2024-03-25T15:34:01Z

We will still have to think about consequences for IO. In particular, Siemens TOF order needed to be handled, so this code will have to be adapted

STIR/src/IO/stir_ecat_common.cxx

Lines 208 to 212 in 8ced2d7

    
           for (int timing_pos_num = 1; timing_pos_num <= max_timing_pos_num; ++timing_pos_num) 
        
             { 
        
               timing_pos_sequence[2 * timing_pos_num - 1] = timing_pos_num; 
        
               timing_pos_sequence[2 * timing_pos_num] = -timing_pos_num; 
        
             }

KrisThielemans · 2024-03-25T15:46:26Z

Should put a check in

STIR/src/buildblock/ProjDataFromStream.cxx

Lines 152 to 155 in 8ced2d7

    
           ProjDataFromStream::set_timing_poss_sequence_in_stream(const std::vector<int>& seq) 
        
           { 
        
             this->timing_poss_sequence = seq; 
        
           }

that values in the seq array are between min and max, to catch people using the "old" convention. (projdatainfo)

KrisThielemans · 2024-03-25T15:58:56Z

#1389 introduced the capability to read TOF bin order from the Interfile header:

STIR/src/IO/InterfileHeader.cxx

Line 621 in 8ced2d7

add_key("TOF bin order", &timing_poss_sequence);

. However, the value of that keyword would now be different.
(Note that #1389 was after the 6.0.0 release, so we are not breaking backwards compatibility w.r.t. a released version).

@NicoleJurjew I don't think this will affect you (as it's independent of the parsing of the Siemens Interfile header), but let us know if it does.

change IO timing pos; add check in ProjDatafromStream; add glossary for TOF number

src/IO/stir_ecat_common.cxx

KrisThielemans · 2024-03-25T21:24:04Z

src/buildblock/ProjDataInfoCylindrical.cxx

  const float z1 = (m_in_mm - max_a * tantheta);
  const float z2 = (m_in_mm - min_a * tantheta);

+  const bool swap = false; // get_tof_delta_time(bin)<0;


actually, this should be deleted, and the whole change w.r.t. master reverted

KrisThielemans · 2024-03-25T21:27:50Z

we got through ctest, but run_test_simulate_and_recon.sh is failing again. 😢

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

nmdicom-recon added 5 commits October 3, 2023 13:51

changing timing index

5bedd93

furter adaptation to timing_pos_num from 0 to num_tof_bin -1, more sw…

5b9d603

…aps found

furter adaptation to timing_pos_num from 0 to num_tof_bin -1, fixed g…

da07d94

…et_k and swap s sym

more tof swaps and fixes for even tof bins

e9ffdd4

merging master and fixing conflicts

c807500

KrisThielemans added this to the v6.0 milestone Jan 15, 2024

KrisThielemans linked an issue Jan 15, 2024 that may be closed by this pull request

TOF: need to support even number of TOF bins and TOF_mashing_factor #1333

Open

danieldeidda commented Jan 17, 2024

View reviewed changes

fixing symmetry test issues

605f950

KrisThielemans requested changes Jan 22, 2024

View reviewed changes

danieldeidda and others added 2 commits January 26, 2024 15:35

fixing asserts and tests

fcfb1ca

Merge remote-tracking branch 'origin/master' into changetofidx

3afc3fa

KrisThielemans mentioned this pull request Jan 27, 2024

require c++-14 #1345

Closed

Merge remote-tracking branch 'origin/master' into changetofidx

5cf36d9

KrisThielemans modified the milestones: v6.0, v6.1 Feb 7, 2024

KrisThielemans reviewed Mar 4, 2024

View reviewed changes

fixing symmetry indeces

ab7c5df

danieldeidda commented Mar 11, 2024

View reviewed changes

src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndProjData.cxx Show resolved Hide resolved

fixing conflincts and merge origin/master

8246115

KrisThielemans mentioned this pull request Mar 14, 2024

add TOF mashing to SSRB #1397

Open

fix Datasymmetry timeshift;

ab23474

change IO timing pos; add check in ProjDatafromStream; add glossary for TOF number

KrisThielemans reviewed Mar 25, 2024

View reviewed changes

src/IO/stir_ecat_common.cxx Outdated Show resolved Hide resolved

KrisThielemans requested changes Mar 25, 2024

View reviewed changes

Update src/IO/stir_ecat_common.cxx

ab7265d

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

KrisThielemans modified the milestones: v6.1, v6.2 May 14, 2024

KrisThielemans modified the milestones: v6.2, v6.3 Jul 9, 2024

KrisThielemans modified the milestones: v6.3, v7.0 Oct 31, 2024

		this->max_num_central_timing_poss_to_process =
		this->proj_data_sptr->get_max_tof_pos_num();

Conversation

danieldeidda commented Jan 15, 2024

Uh oh!

KrisThielemans commented Jan 16, 2024

Uh oh!

danieldeidda commented Jan 16, 2024

Uh oh!

danieldeidda Jan 17, 2024

Choose a reason for hiding this comment

Uh oh!

danieldeidda Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrisThielemans Jan 17, 2024

Choose a reason for hiding this comment

Uh oh!

danieldeidda Jan 17, 2024

Choose a reason for hiding this comment

Uh oh!

KrisThielemans commented Jan 22, 2024

Uh oh!

Uh oh!

Uh oh!

KrisThielemans Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

danieldeidda Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

KrisThielemans Jan 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KrisThielemans commented Jan 27, 2024

Uh oh!

KrisThielemans commented Feb 1, 2024

Uh oh!

danieldeidda commented Feb 2, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danieldeidda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KrisThielemans commented Mar 25, 2024

Uh oh!

KrisThielemans commented Mar 25, 2024 • edited by danieldeidda Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrisThielemans commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

KrisThielemans Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

KrisThielemans commented Mar 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danieldeidda Jan 17, 2024 •

edited

Loading

KrisThielemans commented Mar 25, 2024 •

edited by danieldeidda

Loading

KrisThielemans commented Mar 25, 2024 •

edited

Loading