View offset #181

pwadhwa351 · 2018-05-01T13:11:43Z

No description provided.

NikEfth · 2018-05-01T14:23:51Z

@pw351 When you are finished pls let me know. :)

pwadhwa351 · 2018-05-01T14:30:58Z

I think I am done with the commits. :)

NikEfth · 2018-05-02T10:26:50Z

This is wrong.
It includes parts of my changes. @pwadhwa351 Do the same thing but start from scratch. from a clean master. And do not give the same name twice.

pwadhwa351 · 2018-05-02T10:37:19Z

@NikEfth Can you please specify which changes you are referring to which you made?

NikEfth · 2018-05-02T10:45:26Z

I should not be able to see this, from this branch :

commit b3a955831d9c6b5512e52c4910866e0ab09bfdee
Merge: 124e0889 cca8e608
Author: pw351 <pwadhwa351@gmail.com>
Date:   Fri Apr 27 15:33:02 2018 +0100

    Merge pull request #1 from NikEfth/Viewoffset
    
    Fixed all tests in test_proj_data_info with tilt (view_offset)

KrisThielemans · 2018-05-02T10:48:25Z

@NikEfth, I don't see that commit. In this PR, I see only 4 commits from @pwadhwa351. It restores the PR to what I remember. Possibly you have some links to the old PR? (not sure why thought. this is PR 181, the old one was #130)

NikEfth · 2018-05-02T10:49:11Z

I can see that from
git log

KrisThielemans · 2018-05-02T10:55:28Z

not sure what you did then. you'd have to add a remote, fetch this new branch, and do a git log on that. I guess you might have merged onto an existing branch.
as far as I understand, the commits that I see on github are what's in this branch. (Would be really very surprising otherwise)

NikEfth · 2018-05-02T11:01:34Z

Alright, I see now. so @pwadhwa351 and @pw351 are different accounts with the same brach ... ok

NikEfth · 2018-05-02T15:11:41Z

OK, so let me explain the situation. I made two commits and a PR to Palak. Commit: ed71b4e and 0af0256.
In both make test succeeds but test_simulate_and_recon.sh fail for different reasons.
In the first commit the calculate_attenuation_coefficient fail because it needs --PMRT to run. Iterative reconstructions TESTS THAT USE PROJECTION MATRIX run smoothly and of course analytic fail.
In the second commit I added the PMRT flag but now I get a timeout on the Running precorrections.

I am sorry I will not be able to finish it this week.

KrisThielemans · 2018-05-03T06:47:03Z

I had a look at ed71b4e and added some comments there (not ideal but best I could do). There's a few small things I doubt that can be correct, but the main change here is that the code now takes the offset into account in the translation between views and detectors. I'm not sure if this is the best way though, see also my comment in the code.

I believe that changing the relation between views and detectors is dangerous though. It would have to be taken into account in any code for normalisation etc (detector efficiencies for the first and middle detector would now no longer correspond to the first view). I think that spreads things too far.

Instead, I believe it is better to use the following convention:

"continuous" coordinates such as phi and psi (the one (around the ring") are always in the standard coordinate system (with 0 being up or whatever it is).
"discrete" coordinates in bins always start from 0
we have a phi_offset (aka view_offset) and psi_offset (aka det_num_offset) such that
- phi = phi_offset + view * view_increment
- psi = psi_offset + det_num * det_num_increment

As long as these 2 offsets are consistent, we should be fine. Sadly, because of the interleaving, the relation between these 2 is maybe not trivial, but at least initially I'd vote for psi_offset = 2*phi_offset. That would be backwards compatible for the offset=0 case so probably our only possible choice.

In summary, I think we need a new function get_psi_offset (implemented in terms of the view offset), and used that in ProjDataInfoCylindericalNoArcCorr whereever we convert between psi and detx. I would hope that we don't need any other changes.

src/include/stir/ProjDataInfo.h

KrisThielemans · 2018-05-04T07:12:32Z

I've tried this and can confirm that for the arccorr case, there are round-trip problems, giving for instance

Error. round-trip get_LOR then get_bin (LORAs2Points): segment
        Problem at    segment = -15, axial pos 0, view = 177, tangential_pos_num = -31
        round-trip to segment = 15, axial pos 0, view = -15, tangential_pos_num = 31

showing that the view_num is negative, which shouldn't be the case. so we indeed need some of @NikEfth's fixes.

Previously, when calling the constructors, phi was required to be between 0 and pi, and psi between 0 and 2pi. This is no longer the case.

- make sure angles are first converted to a 0..2pi range - introduce get_psi_offset for detector angles along the ring to be consistent with view_offset (view_num=0 still corresponds to det_a==0)

revert to original version but using get_scanner_ptr() in derived classes

KrisThielemans · 2018-05-05T09:03:41Z

I've now added a few "modulo" operations (to_0_2pi) and then the get_psi_offset works. All test now work, except the analytic reconstructions as they use the incremental interpolating backprojector which doesn't take this into account. that'll be a pain.

thanks @NikEfth for the discussion on this!

src/include/stir/LORCoordinates.inl

KrisThielemans · 2020-05-28T07:21:10Z

shifted to 5.0 as output will be incompatible for some scanners (with non-zero view-offset)

Also moved lines from ocumentation/release_3.1.htm to 5.0.htm

If intrinsic tilt would be very large or somehow bring the phi outside the range, the previous code presumably could go wrong

The interpolating backprojector doesn't support view-offset yet, so FBP tests failed. We now run them on data with view-offset = 0.

KrisThielemans · 2020-12-19T14:04:35Z

I've "fixed" the tests by running FBP on data with view-offset = 0. I expect this to work now.

Things to do:

check if the scatter code does the proper thing
make CMake option to disable view-offset, or change release notes

circumvent limitation of the interpolating backprojector

now called get_intrinsic_azimuthal_tilt()

…F) [ci skip]

start-angle was supported for SPECT already long time ago.

When adding views, the view-offset should be adjusted to the average of the mashed views, otherwise the image will be rotated.

As we now take view-mashing into account in view-offset, run_tests.sh was failing as it compares with old reconstructions. So we change the header to compensate.

The old code doesn't handle it, so we check upfront. Also removed it from the recon_test_pack.

With the merge of UCL/STIR#181 and the checks performed in #59, an offset of -8 will no longer work in STIR.

pw351 added 3 commits May 1, 2018 13:34

View Offset Enabled

03ea634

View Offset test Data Symmetries OK

e2d2a65

test_proj_data_info

d4f19a1

test_proj_data_info author and copyrights

0ae8313

KrisThielemans reviewed May 4, 2018

View reviewed changes

src/include/stir/ProjDataInfo.h Outdated Show resolved Hide resolved

KrisThielemans added 4 commits May 5, 2018 07:26

make some LORCoordinates safe for non-standard ranges for phi

ee9082e

Previously, when calling the constructors, phi was required to be between 0 and pi, and psi between 0 and 2pi. This is no longer the case.

corrected view-offset handling

5229035

- make sure angles are first converted to a 0..2pi range - introduce get_psi_offset for detector angles along the ring to be consistent with view_offset (view_num=0 still corresponds to det_a==0)

Merge branch 'master' into ViewOffset

bf20429

make scanner_ptr member of ProjDataInfo private again

78f1e23

revert to original version but using get_scanner_ptr() in derived classes

NikEfth reviewed May 5, 2018

View reviewed changes

src/include/stir/LORCoordinates.inl Show resolved Hide resolved

KrisThielemans added the in-progress label May 12, 2018

KrisThielemans added this to the v4.1 milestone May 23, 2020

KrisThielemans mentioned this pull request May 28, 2020

GE RDF HDF5 remaining issues #555

Open

7 tasks

KrisThielemans modified the milestones: v4.1, v5.0 May 28, 2020

KrisThielemans mentioned this pull request Jun 1, 2020

D690 Simulations will not unlist UCL/STIR-GATE-Connection#14

Closed

KrisThielemans mentioned this pull request Nov 19, 2020

GE D690 flipped images #756

Open

KrisThielemans added 4 commits December 19, 2020 10:32

Merge branch 'master' into ViewOffset

698e994

Also moved lines from ocumentation/release_3.1.htm to 5.0.htm

added another to_0_2pi to be safe in phi/view calculations

8ad9379

If intrinsic tilt would be very large or somehow bring the phi outside the range, the previous code presumably could go wrong

let interpolating backprojector abort if view-offset non-zero

d4c2d65

updated test scripts for view-offset

dea210a

The interpolating backprojector doesn't support view-offset yet, so FBP tests failed. We now run them on data with view-offset = 0.

run test_FBP* with view-offset=0

ebe4c9e

circumvent limitation of the interpolating backprojector

KrisThielemans mentioned this pull request Mar 2, 2021

ROOT detector offset #837

Closed

Merge branch 'master' into ViewOffset

af38f6d

KrisThielemans force-pushed the ViewOffset branch from d6ee4d3 to 60d6af8 Compare March 4, 2021 20:01

KrisThielemans added 3 commits March 4, 2021 20:50

added Scanner::>get_names_of_predefined_scanners

8b64889

renamed Scanner::get/set_default_intrinsic_tilt()

726ec75

now called get_intrinsic_azimuthal_tilt()

[CMake] added STIR_LEGACY_IGNORE_VIEW_OFFSET option (defaulting to OF…

b9c6e7f

…F) [ci skip]

KrisThielemans force-pushed the ViewOffset branch from 60d6af8 to b9c6e7f Compare March 4, 2021 20:53

KrisThielemans added 7 commits March 5, 2021 08:32

clarified release notes on view offset

0bbe0cc

start-angle was supported for SPECT already long time ago.

removed some unused variables

dc696ae

Take view-mashing into account in view-offset

134d432

When adding views, the view-offset should be adjusted to the average of the mashed views, otherwise the image will be rotated.

clean-up of copyright and documentation on view offset

bd3f8f1

Modify recon_test_pack/Utahscat600k_ca_seg4.hs for view-offset

32c4e6f

As we now take view-mashing into account in view-offset, run_tests.sh was failing as it compares with old reconstructions. So we change the header to compensate.

ForwardProjectorByBinUsingRayTracing calls error() for non-zero offset

f8410c8

The old code doesn't handle it, so we check upfront. Also removed it from the recon_test_pack.

correct copy-paste mistake

ee6e7c6

KrisThielemans merged commit 4b36d66 into UCL:master Mar 5, 2021

KrisThielemans added enhancement and removed in-progress labels Mar 11, 2021

robbietuk mentioned this pull request Mar 15, 2021

Change the D690 offset (num of detectors) := 0 UCL/STIR-GATE-Connection#62

Merged

robbietuk pushed a commit to UCL/STIR-GATE-Connection that referenced this pull request Mar 15, 2021

Merge pull request #62 from UCL/ML_norm/D690offset_change

d97a320

With the merge of UCL/STIR#181 and the checks performed in #59, an offset of -8 will no longer work in STIR.

KrisThielemans mentioned this pull request Apr 12, 2021

block and generic scanner geometry #577

Merged

robbietuk mentioned this pull request Sep 27, 2021

Listmode reconstruction with bin normalisation causes rotation of image xy for data but not sensitivity #407

Open

ashgillman mentioned this pull request Oct 14, 2021

Documenting tof branch merge conflicts #943

Closed

robbietuk mentioned this pull request Feb 25, 2022

Rotational symmetry tests #1002

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

View offset #181

View offset #181

pwadhwa351 commented May 1, 2018

NikEfth commented May 1, 2018

pwadhwa351 commented May 1, 2018

NikEfth commented May 2, 2018

pwadhwa351 commented May 2, 2018

NikEfth commented May 2, 2018 •

edited

Loading

KrisThielemans commented May 2, 2018

NikEfth commented May 2, 2018

KrisThielemans commented May 2, 2018

NikEfth commented May 2, 2018

NikEfth commented May 2, 2018 •

edited

Loading

KrisThielemans commented May 3, 2018

KrisThielemans commented May 4, 2018

KrisThielemans commented May 5, 2018

KrisThielemans commented May 28, 2020

KrisThielemans commented Dec 19, 2020 •

edited

Loading

View offset #181

View offset #181

Conversation

pwadhwa351 commented May 1, 2018

NikEfth commented May 1, 2018

pwadhwa351 commented May 1, 2018

NikEfth commented May 2, 2018

pwadhwa351 commented May 2, 2018

NikEfth commented May 2, 2018 • edited Loading

KrisThielemans commented May 2, 2018

NikEfth commented May 2, 2018

KrisThielemans commented May 2, 2018

NikEfth commented May 2, 2018

NikEfth commented May 2, 2018 • edited Loading

KrisThielemans commented May 3, 2018

KrisThielemans commented May 4, 2018

KrisThielemans commented May 5, 2018

KrisThielemans commented May 28, 2020

KrisThielemans commented Dec 19, 2020 • edited Loading

NikEfth commented May 2, 2018 •

edited

Loading

NikEfth commented May 2, 2018 •

edited

Loading

KrisThielemans commented Dec 19, 2020 •

edited

Loading