block and generic scanner geometry #577

pkhateri · 2020-06-12T12:24:54Z

new classes and changes for accurate scanner geometry

list of modified classes: - InterfileHeader - interfile.cxx - Scanner - VoxelsOnCartesianGrid - LORCoordinates - CListRecordSAFIR - DataSymmetriesForBins_PET_CartesianGrid - ProjMatrixByBinUsingRayTracing list of added classes: - GeometryBlocksOnCylindrical - ProjDataInfoBlocksOnCylindrical - ProjDataInfoBlocksOnCylindricalNoArcCorr

KrisThielemans · 2020-06-15T16:14:51Z

https://travis-ci.org/github/UCL/STIR/jobs/697593231#L3130-3142 says

In file included from /home/travis/build/UCL/STIR/src/include/stir/make_array.h:209:0,
                 from /home/travis/build/UCL/STIR/src/include/stir/GeometryBlocksOnCylindrical.h:35,
                 from /home/travis/build/UCL/STIR/src/include/stir/ProjDataInfoBlocksOnCylindricalNoArcCorr.h:32,
                 from /home/travis/build/UCL/STIR/src/include/stir/listmode/CListRecordSAFIR.inl:33,
                 from /home/travis/build/UCL/STIR/src/include/stir/listmode/CListRecordSAFIR.h:261,
                 from /home/travis/build/UCL/STIR/src/include/stir/IO/SAFIRCListmodeInputFileFormat.h:47,
                 from /home/travis/build/UCL/STIR/src/IO/IO_registries.cxx:67:
/home/travis/build/UCL/STIR/src/include/stir/make_array.inl: In instantiation of ‘stir::Array<(num_dimensions + 1), T> 
stir::make_array(const stir::Array<num_dimensions, T>&, const stir::Array<num_dimensions, T>&, const stir::Array<num_dimensions, T>&) [with int num_dimensions = 1; T = float]’:

/home/travis/build/UCL/STIR/src/include/stir/GeometryBlocksOnCylindrical.inl:39:7:   required from here

/home/travis/build/UCL/STIR/src/include/stir/make_array.inl:255:113: error: conversion from ‘stir::NumericVectorWithOffset<stir::Array<1, float>, float>’ to non-scalar type ‘const stir::Array<2, float>’ requested

   const Array<num_dimensions+1,T> a = NumericVectorWithOffset<Array<num_dimensions,T>,T>(make_vector(a0, a1, a2));

make[2]: *** [src/swig/CMakeFiles/_stir.dir/__/IO/IO_registries.cxx.o] Error 1

Not sure why all the previous IO_registries compilations worked though.

KrisThielemans · 2020-06-25T11:54:20Z

@pkhateri I believe I fixed the problem in #605 . If you merge STIR master in here, that might work.

Potentially I could do it for you, but I'd be pushing to your master branch apparently. 9always best to use a branch for a PR in the future).

KrisThielemans · 2020-07-11T13:08:26Z

same problem when compiler _stir.dir/IO_registries related to SWIG strangely enough.
https://travis-ci.org/github/UCL/STIR/jobs/707135030#L3168

/home/travis/build/UCL/STIR/src/include/stir/make_array.inl: In instantiation of ‘stir::Array<(num_dimensions + 1), T> stir::make_array(const stir::Array<num_dimensions, T>&, const stir::Array<num_dimensions, T>&, const stir::Array<num_dimensions, T>&) [with int num_dimensions = 1; T = float]’:
/home/travis/build/UCL/STIR/src/include/stir/GeometryBlocksOnCylindrical.inl:39:7:   required from here
/home/travis/build/UCL/STIR/src/include/stir/make_array.inl:255:113: error: conversion from ‘stir::NumericVectorWithOffset<stir::Array<1, float>, float>’ to non-scalar type ‘const stir::Array<2, float>’ requested
   const Array<num_dimensions+1,T> a = NumericVectorWithOffset<Array<num_dimensions,T>,T>(make_vector(a0, a1, a2));

- moved some inline functions to .cxx. They were either not used frequently or probably too big to be inlined - made some member functions const and pass "bigger" arguments as references - avoid some warnings about double->float conversion The first item resolves a problem with SWIG compilation UCL#604 by working around it.

They aren't used, and probably more confusing than anything else.

KrisThielemans

@pkhateri I've resolved the SWIG compilation error. However, there are now a lot of failures in the tests. Many of them just state

ERROR: Crystal Map not defined!

Python tests show

RuntimeError: ProjDataInfoGeneric::get_ring_pair_for_segment_axial_pos_num does not work for data with axial compression

implying that ProjDataInfoGeneric is called for "normal" interfile headers.

I do not understand your intentions with this PR well enough to solve this. Can you help?

KrisThielemans · 2020-07-13T18:11:55Z

src/include/stir/listmode/CListRecordSAFIR.h

@@ -42,7 +44,7 @@
 #include "boost/cstdint.hpp"

 #include "stir/listmode/DetectorCoordinateMapFromFile.h"
-
+#include "boost/make_shared.hpp"


not needed. You can use either MAKE_SHARED (or even std::make_shared now that we're C++11).

KrisThielemans · 2020-07-14T06:34:00Z

Example Travis test log https://travis-ci.org/github/UCL/STIR/jobs/707731577

…when the scanner geometry is not defined in the header

pkhateri · 2020-07-16T15:15:39Z

@pkhateri I've resolved the SWIG compilation error. However, there are now a lot of failures in the tests. Many of them just state
ERROR: Crystal Map not defined!
Python tests show
RuntimeError: ProjDataInfoGeneric::get_ring_pair_for_segment_axial_pos_num does not work for data with axial compression
implying that ProjDataInfoGeneric is called for "normal" interfile headers.

I do not understand your intentions with this PR well enough to solve this. Can you help?

I pushed some fixes to the repository. Now all the tests should pass.
The modifications were originally applied into an old version (from ~3 yrs ago). When I merged that version to this new one, not all changes were copied. So I added them manually now. Hopefully, there's no problem any more. Otherwise please let me know.
I also added some new utilities...

KrisThielemans · 2020-10-23T08:57:15Z

thanks @pkhateri sorry for the delay. ctest is indeed happy. great. Unfortunately there were still quite a lot of errors with the recon_test_pack. You could have a look at https://travis-ci.org/github/UCL/STIR/jobs/712819897 but will just have to run the scripts yourself to figure out what they are, as Travis doesn't show us the logs at the moment. Feel free to ask questions of course.

Thanks for the extra utilities. Could you add some brief info on them in the STIR_UsersGuide.tex? Ideally also a bit more documentation in their headers (doxygen) if it isn't immediately obvious what they do. I'm reluctant to add the following though:

find_counts_in_cylindrical_ROI.cxx can be done via list_ROI_values I guess
convert_projdata_types.cxx seems unnecessary. I believe it just makes a copy, but you can do that with stir_math -s copy orig
trim_projdata.cxx seems unnecessary: SSRB -t <trim> out in 1 should do the same

Please also update documentation/release_5.0.htm.

created text for voxel on cartesian grid

as opposed -VV, we only output failed tests now, preventing log overrun [actions skip]

KrisThielemans · 2022-02-16T22:11:17Z

@danieldeidda Appveyor builds are all failing on test_blocks_on_cylindrical with an exception, see bottom of
https://ci.appveyor.com/project/KrisThielemans/stir/builds/42581277/job/lq3sb3pvh81g31gp

It runs fine on my Visual Studio, so I suspect this it hits a memory limit, as these tests use ~6GB. I think best to use enable_caching(false) on the matrices. Also, the compilation above of test_blocks_on_cylindrical.cxx shows quite a few warnings. Maybe you can kill a few?

danieldeidda · 2022-02-17T08:20:47Z

@danieldeidda Appveyor builds are all failing on test_blocks_on_cylindrical with an exception, see bottom of https://ci.appveyor.com/project/KrisThielemans/stir/builds/42581277/job/lq3sb3pvh81g31gp

It runs fine on my Visual Studio, so I suspect this it hits a memory limit, as these tests use ~6GB. I think best to use enable_caching(false) on the matrices. Also, the compilation above of test_blocks_on_cylindrical.cxx shows quite a few warnings. Maybe you can kill a few?

I think we can remove the following:
WARNING: Disabling all symmetries except for symmtery_z since they are not implemented in block geometry yet.

as the symmetry_z is also disabled

KrisThielemans · 2022-02-17T08:31:21Z

I guess we cannot remove it, but have to rephrase it to "all symmetries disabled".

However, I was talking about compilation warnings (as you can see in the Appveyor log). The MS VC compiler issues far more warnings than gcc...

danieldeidda · 2022-02-17T10:03:50Z

the all seem to be conversion from 'double' to 'float'

Anyway do you want the scatter changes to be pushed here or we merge to master first?

KrisThielemans · 2022-02-17T15:25:30Z

the all seem to be conversion from 'double' to 'float'

yup. needs some (ugly) static_cast<float>(xxx)

Anyway do you want the scatter changes to be pushed here or we merge to master first?

I'd like to merge this to master first.

…or symmetries

danieldeidda · 2022-02-18T11:01:43Z

This should be my last commit for this PR I guess

KrisThielemans

thanks. A few more unfortunately. sorry

src/include/stir/ProjDataInfoGeneric.inl

src/test/test_proj_data_info.cxx

src/recon_test/test_blocks_on_cylindrical_projectors.cxx

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

KrisThielemans · 2022-02-18T15:05:08Z

can you fix white-spacing any the test_blocks* file, and any other file you created in this PR, and also fix its permissions

cd STIR/src/recon_test
clang-format -i test_blocks_on_cylindrical_projectors.cxx
chmod -x test_blocks_on_cylindrical_projectors.cxx

danieldeidda · 2022-02-18T15:44:06Z

OK I think I have done all the corrections

KrisThielemans

more things to do, but we'll merge this now as it's in good shape.

KrisThielemans

sorry. hit the wrong button!

KrisThielemans · 2022-02-20T01:03:30Z

@pkhateri This is merged now. Thanks a lot! I've sent an email to the list, but don't seem to have an email address for you anymore. I hope that this messages reaches you. All the best.

Parisa Khateri and others added 9 commits August 20, 2018 15:07

Implementation of the generic geometry

4c62a51

crystal map only asked if 'Generic' and built-in error for maps

b8c92a0

corrections for issue 1

cf4340e

issue #2 first patch of corrections

616d8c6

Update 2 for issue 2

db527c0

megre devBlock with ucl/master and resolve conflicts

214fdb5

add license

f7df3b9

Merge branch 'master' into master

2e71b94

This was referenced Jun 24, 2020

swig files compilation error #604

Closed

Doxygen corrections #605

Merged

Merge branch 'master' into block_projectors

c76f3fa

KrisThielemans added 2 commits July 13, 2020 07:36

removed some commented out lines in the SAFIR OSMAPOSL .par file

38cd06b

They aren't used, and probably more confusing than anything else.

KrisThielemans reviewed Jul 14, 2020

View reviewed changes

KrisThielemans added the in-progress label Jul 14, 2020

KrisThielemans mentioned this pull request Jul 14, 2020

LOR randomization in SAFIR input file format #507

Merged

Parisa Khateri added 4 commits July 16, 2020 12:26

fix bug: set default scanner geometry as cylindrical to avoid errors …

7f0794a

…when the scanner geometry is not defined in the header

add SAFIRDualRingPrototype to the list of defined scanners

db65ef2

small leftovers from block/generic implementation

c948ae2

add new utilities

57a54f0

Parisa Khateri and others added 3 commits July 16, 2020 17:36

small fix

a311276

fix a bug

097977b

Merge branch 'master' into block_projectors

1fc463a

danieldeidda and others added 6 commits January 25, 2022 14:30

[skip ci] modify tests documentation

656889a

fixing error with negative view offset

5147f7b

created text for voxel on cartesian grid

ci-skip changed name of test

e9e6990

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

5284c24

[Appveyor] run ctest with --output-on-failure

d0f26d7

as opposed -VV, we only output failed tests now, preventing log overrun [actions skip]

reduced verbosity and warnings in test_blocks_on_cylindrical_projectors

20bfaeb

danieldeidda added 3 commits February 17, 2022 16:58

remove repetition code in test by using a function, change warning f…

95af0dd

…or symmetries

disabling cache

e50aeb0

kill warnings and correct typo for enable_cache(false)

50c0fee

KrisThielemans requested changes Feb 18, 2022

View reviewed changes

danieldeidda and others added 2 commits February 18, 2022 13:33

Update src/include/stir/ProjDataInfoGeneric.inl

894842f

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

Update src/include/stir/ProjDataInfoGeneric.inl

7e75c24

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

danieldeidda added 2 commits February 18, 2022 15:37

clean-up repetition by using template function and fix white spacing

026ba19

Merge branch 'master' of https://github.com/pkhateri/STIR into getm_fix

b6878e5

update release notes for block feature [ci skip]

535c5ce

KrisThielemans requested changes Feb 19, 2022

View reviewed changes

KrisThielemans approved these changes Feb 19, 2022

View reviewed changes

KrisThielemans merged commit bbe9f5a into UCL:master Feb 19, 2022

KrisThielemans added enhancement and removed in-progress labels Feb 20, 2022

KrisThielemans added this to the v5.0 milestone Feb 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

block and generic scanner geometry #577

block and generic scanner geometry #577

pkhateri commented Jun 12, 2020

KrisThielemans commented Jun 15, 2020

KrisThielemans commented Jun 25, 2020

KrisThielemans commented Jul 11, 2020

KrisThielemans left a comment

KrisThielemans Jul 13, 2020

KrisThielemans commented Jul 14, 2020

pkhateri commented Jul 16, 2020

KrisThielemans commented Oct 23, 2020

KrisThielemans commented Feb 16, 2022

danieldeidda commented Feb 17, 2022

KrisThielemans commented Feb 17, 2022

danieldeidda commented Feb 17, 2022

KrisThielemans commented Feb 17, 2022

danieldeidda commented Feb 18, 2022

KrisThielemans left a comment

KrisThielemans commented Feb 18, 2022

danieldeidda commented Feb 18, 2022

KrisThielemans left a comment

KrisThielemans left a comment

KrisThielemans commented Feb 20, 2022

block and generic scanner geometry #577

block and generic scanner geometry #577

Conversation

pkhateri commented Jun 12, 2020

KrisThielemans commented Jun 15, 2020

KrisThielemans commented Jun 25, 2020

KrisThielemans commented Jul 11, 2020

KrisThielemans left a comment

Choose a reason for hiding this comment

KrisThielemans Jul 13, 2020

Choose a reason for hiding this comment

KrisThielemans commented Jul 14, 2020

pkhateri commented Jul 16, 2020

KrisThielemans commented Oct 23, 2020

KrisThielemans commented Feb 16, 2022

danieldeidda commented Feb 17, 2022

KrisThielemans commented Feb 17, 2022

danieldeidda commented Feb 17, 2022

KrisThielemans commented Feb 17, 2022

danieldeidda commented Feb 18, 2022

KrisThielemans left a comment

Choose a reason for hiding this comment

KrisThielemans commented Feb 18, 2022

danieldeidda commented Feb 18, 2022

KrisThielemans left a comment

Choose a reason for hiding this comment

KrisThielemans left a comment

Choose a reason for hiding this comment

KrisThielemans commented Feb 20, 2022