Major fix for get_s for BlocksOnCylindrical along with with minor changes #2

VietAnhDao · 2021-10-06T11:28:27Z

No description provided.

…nges

VietAnhDao · 2021-10-06T11:30:25Z

@KrisThielemans @danieldeidda @emikhay

src/buildblock/SSRB.cxx

src/buildblock/GeometryBlocksOnCylindrical.cxx

src/buildblock/SSRB.cxx

src/include/stir/ProjDataInfoBlocksOnCylindrical.inl

src/scatter_buildblock/ScatterSimulation.cxx

src/scatter_buildblock/scatter_detection_modelling.cxx

KrisThielemans · 2021-10-06T12:36:09Z

There's still some gremlins here. In addition, there's too many white-space changes. I'd prefer that you undo them. This PR should be as small as possible. At the moment, I have to append ?w=1 to the URL in the review as otherwise I'm overwhelmed. Even then there are quite a few changes that have nothing to do with code.

We'll solve the formatting after merging the PRs.

VietAnhDao · 2021-10-11T16:34:30Z

Scatter estimation & some of SSRB changes was reverted/deleted. Previously changes was made so BoC projection data was forced to be accepted and it is better to undo these changes. As a consequence scatter estimation will now have break segmentation error if ran with BoC data.
Scatter simulation still works fine.

KrisThielemans

still some white-space diffs left unfortunately.

Main thing is that the SSRB code doesn't look correct

src/buildblock/SSRB.cxx

KrisThielemans · 2021-10-11T19:50:47Z

src/scatter_buildblock/ScatterSimulation.cxx

    error(boost::format("ScatterSimulation: limitation in #planes and voxel-size for the %1% image.\n"
                        "This would cause a shift of %2%mm w.r.t. the activity image.\n"
                        "(see https://github.com/UCL/STIR/issues/495.")
-          % name % (z_to_middle - z_to_middle_standard));
+           % name % (z_to_middle - z_to_middle_standard));


white space

KrisThielemans · 2021-10-11T19:51:17Z

src/scatter_buildblock/ScatterSimulation.cxx

        else if (!is_null_ptr(proj_data_info_cyl_noarc_cor_sptr))
 	  {
 	    const float total_axial_length = proj_data_info_cyl_noarc_cor_sptr->get_scanner_sptr()->get_num_rings() *
 	      proj_data_info_cyl_noarc_cor_sptr->get_scanner_sptr()->get_ring_spacing();
-


white space

KrisThielemans · 2021-10-11T19:51:22Z

src/scatter_buildblock/ScatterSimulation.cxx

    // Find how much is the delta ring
    // If the previous projdatainfo had max segment == 1 then should be from SSRB
    // in ScatterEstimation. Otherwise use the max possible.
    int delta_ring = proj_data_info_cyl_noarc_cor_sptr->get_num_segments() == 1 ?  0 :
            new_scanner_sptr->get_num_rings()-1;
-


white space

src/scatter_buildblock/scatter_detection_modelling.cxx

VietAnhDao · 2021-10-25T12:57:39Z

Completely reverted SSRB code. A proper implementation for SSRB will be given in separate PR.

KrisThielemans · 2021-10-29T07:23:29Z

src/recon_buildblock/ProjMatrixByBinUsingRayTracing.cxx

+        if (proj_data_info_ptr->get_scanner_ptr()->get_scanner_geometry() == "BlocksOnCylindrical")
+  {
+    fovrad_in_mm =
+        min((min(max_index.x(), -min_index.x()) - 5.f) * voxel_size.x(),
+            (min(max_index.y(), -min_index.y()) - 5.f) * voxel_size.y());
+  }


can you document why you need this?

reducing the fovrad_in_mm prevent memory leak. Due to radius problem in "BlocksOnCylindrical" it might perform ray tracing where voxels doesn't exist. This might be the source of memory leak. I don't know too much about C++ technicality behind this but I know that if I perform reconstruction without -5.f or use +5.f then memory leak will occur. This I have tested.

KrisThielemans · 2021-11-08T15:14:47Z

will need to add some code to test_ScatterSimulation to run the test with a block scanner. Likely needs adding code for the downsampling of such a scanner.

KrisThielemans · 2021-11-08T15:04:08Z

src/scatter_buildblock/ScatterSimulation.cxx

+            // BlocksOnCylindrical geometry is already align no shifted needed.
+            this->shift_detector_coordinates_to_origin =
+                CartesianCoordinate3D<float>(0,0,0);


In @danieldeidda's change, we shifted the "blocks" geometry to coincide with the cylindrical one. (i.e. 0 in first ring), so we will need to replace this code with the same as the lines above

KrisThielemans · 2021-11-08T15:57:25Z

src/recon_buildblock/ProjMatrixByBinUsingRayTracing.cxx

+  if (costheta < 0.0001) {
+    costheta = 0.0001;
+  }


Here is what we think for get_tantheta() independent of R

CartesianCoordinate3D<float> _p1; CartesianCoordinate3D<float> _p2; find_cartesian_coordinates_of_detection(_p1, _p2, bin); CartesianCoordinate3D<float> p2_minus_p1 = _p2 - _p1; return p2_minus_p1.z() / (sqrt(square(p2_minus_p1.x())+square(p2_minus_p1.y());

src/recon_buildblock/ProjMatrixByBinUsingRayTracing.cxx

src/include/stir/ProjDataInfoBlocksOnCylindricalNoArcCorr.inl

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

KrisThielemans · 2021-11-08T17:28:52Z

Thanks! Sadly, we cannot merge as there is a conflict in src/buildblock/GeometryBlocksOnCylindrical.cxx. git cannot resolve them most likely due to white-space changes (which is one reason I don't like them :-)).

In particular, start_z has been changed by @danieldeidda.

Can you merge pkhateri/master on yours and sort out the conflict? sorry.

KrisThielemans · 2021-11-08T21:59:06Z

src/recon_buildblock/ProjMatrixByBinUsingRayTracing.cxx

@@ -668,6 +669,7 @@ calculate_proj_matrix_elems_for_one_bin(

      phi = proj_data_info_noarccor.get_phi(bin);
      s_in_mm = proj_data_info_noarccor.get_s(bin);
+      tantheta = proj_data_info_noarccor.get_tantheta(bin);


why is it repeated here? You've already set it above. (in fact, s_in_mm is also set above)

it was set before I downcast the projection data so it would be accessible outside if statement and repeated inside the if statement to get correct value from each geometry of projection data.

get_tantheta is virtual, so downcasting or not makes no difference: it will call the same function.

didn't know this. What would happen if there is multiple get_tantheta() for each geometry?

The pointer points to one particular object. Calling one of its virtual functions will call the one from its type, even if the pointer is from a base type. "run-time polymorphism" :-)

KrisThielemans

I found 2 more things. sorry.

I also noticed you marked several of my requests to reinstate white-space/comment to the original lines as "resolved", but they are still there. Not sure why you did that. However, this is a minor thing, so feel free to ignore.

KrisThielemans · 2021-11-08T22:04:42Z

src/recon_buildblock/ProjMatrixByBinUsingRayTracing.cxx

+
+  float costheta = 1/sqrt(1+square(tantheta));


in fact, these lines can now be reinstated, and all the tantheta lines above removed, which will lead to a smaller change.

Yeah sorry, I thought I made the changes to fix the white space problem so I marked as resolve. Not too sure if it's me or you who should be marking them as resolved, if it's not resolved just mark them as unresolved again.

As for the git conflict, do I do git merge master? My plan is probably add Daniels changes then delete the "X" orientation code after.

KrisThielemans · 2021-11-08T22:30:06Z

As for the git conflict, do I do git merge master? My plan is probably add Daniels changes then delete the "X" orientation code after.

what you should normally do is (assuming that you called the remote pointing to Parisa's fork pkatheri)

git fetch pkhateri
git checkout BlockSTIR # just to make sure
git merge pkhateri/master
# resolve conflict by editing src/buildblock/GeometryBlocksOnCylindrical.cxx
git add src/buildblock/GeometryBlocksOnCylindrical.cxx
git commit

This will pull in Daniel's new test but also the conflicting edits, which is why you need to resolve them. (i.e. no need to remove the "X" configuration again)

KrisThielemans · 2021-11-08T22:32:56Z

I thought I made the changes to fix the white space problem so I marked as resolve. Not too sure if it's me or you who should be marking them as resolved, if it's not resolved just mark them as unresolved again.

Looks like I cannot "unresolve". Have a look at the diff in ScatterEstimation.cxx.

KrisThielemans · 2021-11-09T08:04:01Z

src/buildblock/GeometryBlocksOnCylindrical.cxx

 		see start_x*/

 		//calculate start_point to build the map.
+<<<<<<< HEAD


well. no. You've now committed the file that contains the conflict, i.e. it has both versions (your code here, and the original pkhateri/master code below). You need to manually edit the file.

ok figured out what happened. VS code trying to be helpful shows the conflict as an overlay and some options to resolve the conflict. Unfortunately, unless I select an option it will keep both changes which is what it did :) .

KrisThielemans · 2021-11-09T10:35:23Z

great. thanks. Can you sort out the tantheta comments in src/recon_buildblock/ProjMatrixByBinUsingRayTracing.cxx. Possibly the white-space/comment stuff in the scatter code. Then ready to go!

KrisThielemans · 2021-11-09T10:45:45Z

@danieldeidda @emikhay this is nearly ready. Any objections from anyone to squash-merge (as opposed to merge) when done? The disadvantage is that it gets you into trouble if you've merged this PR somewhere else already. The advantage however is that it removes a lot of the back and forth commits, so I'd prefer that.

If no problem, @VietAnhDao it'd help if you draft a commit message for the squash-merge, i.e. a few bullets with the actual changes that survived.

- s coodrinate is derived from vectors. - tantheta is derived from vectors. - scatter simulation accept BlocksOnCylindrical.

VietAnhDao · 2021-11-09T11:13:42Z

done :) . I hope I've removed all the white space.

KrisThielemans · 2021-11-09T11:19:09Z

I hope I've removed all the white space.

almost, but fine 😄

- s coodrinate is derived from vectors. - tantheta is derived from vectors. - scatter simulation accept BlocksOnCylindrical. - remove X orientation.

VietAnhDao · 2021-11-09T11:20:05Z

I have now :)

KrisThielemans · 2021-11-09T11:41:12Z

Summary of the changes:

get_s() and get_tantheta() is derived from detection coordinates for the BlocksOnCylindrical case
scatter simulation accept BlocksOnCylindrical.

KrisThielemans · 2021-11-09T11:44:29Z

Now I find that I cannot merge this...

@pkhateri could you squash-merge with the above commit message?

KrisThielemans · 2021-11-09T11:49:32Z

@pkhateri or give me write permission to your master branch.

- get_s and get_tantheta are derived from end-points. - scatter simulation accept BlocksOnCylindrical. - remove X orientation. squashed commit of #2 Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>

KrisThielemans · 2021-11-09T21:23:02Z

I've squash-merged this on the git command line and pushed to (this) master. So this PR should be closed (but I cannot).

Thanks @VietAnhDao !

VietAnhDao · 2021-11-09T21:30:47Z

Nice ! bit messy but worked out in the end :) do you want me to close this? I do have the "Close with comment" option.

KrisThielemans · 2021-11-10T09:38:50Z

yes please

VietAnhDao · 2021-11-10T10:31:08Z

Pull request accepted.

Major fix for get_s for BlocksOnCylindrical along with with minor cha…

d60ba18

…nges

KrisThielemans suggested changes Oct 6, 2021

View reviewed changes

KrisThielemans mentioned this pull request Oct 6, 2021

superblocks-case update in find_ML_normfactors3D.cxx and apply_normfactors3D.cxx #3

Closed

VietAnhDao added 4 commits October 8, 2021 14:08

remove comments

532e35b

undo & remove comment

5c683af

remove comment

30a0696

fixes and correct formating

bb84058

KrisThielemans suggested changes Oct 11, 2021

View reviewed changes

VietAnhDao added 2 commits October 25, 2021 10:51

undo SSRB changes, will be a seperate PR.

3be53d2

minor fix

28b3c67

KrisThielemans reviewed Oct 29, 2021

View reviewed changes

KrisThielemans suggested changes Nov 8, 2021

View reviewed changes

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

VietAnhDao and others added 2 commits November 8, 2021 16:36

Update src/include/stir/ProjDataInfoBlocksOnCylindricalNoArcCorr.inl

91c90c3

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

Update src/recon_buildblock/ProjMatrixByBinUsingRayTracing.cxx

799b29c

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

VietAnhDao added 2 commits November 8, 2021 20:59

chage get_tantheta()

7f2b64e

semicolon

019b3bc

KrisThielemans reviewed Nov 8, 2021

View reviewed changes

KrisThielemans suggested changes Nov 8, 2021

View reviewed changes

merged master to BlocksSTIR

b938023

KrisThielemans suggested changes Nov 9, 2021

View reviewed changes

danieldeidda approved these changes Nov 9, 2021

View reviewed changes

remove confict

ed6b62e

Summary of chages:

629ff07

- s coodrinate is derived from vectors. - tantheta is derived from vectors. - scatter simulation accept BlocksOnCylindrical.

Summary of chages:

27eea7c

- s coodrinate is derived from vectors. - tantheta is derived from vectors. - scatter simulation accept BlocksOnCylindrical. - remove X orientation.

KrisThielemans approved these changes Nov 9, 2021

View reviewed changes

VietAnhDao closed this Nov 10, 2021

Major fix for get_s for BlocksOnCylindrical along with with minor changes #2

Major fix for get_s for BlocksOnCylindrical along with with minor changes #2

Conversation

VietAnhDao commented Oct 6, 2021

VietAnhDao commented Oct 6, 2021

KrisThielemans commented Oct 6, 2021

VietAnhDao commented Oct 11, 2021 • edited Loading

KrisThielemans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VietAnhDao commented Oct 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented Nov 8, 2021 • edited Loading

KrisThielemans commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented Nov 9, 2021

KrisThielemans commented Nov 9, 2021

VietAnhDao commented Nov 9, 2021

KrisThielemans commented Nov 9, 2021

VietAnhDao commented Nov 9, 2021

KrisThielemans commented Nov 9, 2021

KrisThielemans commented Nov 9, 2021

KrisThielemans commented Nov 9, 2021

KrisThielemans commented Nov 9, 2021

VietAnhDao commented Nov 9, 2021 • edited Loading

KrisThielemans commented Nov 10, 2021

VietAnhDao commented Nov 10, 2021

VietAnhDao commented Oct 11, 2021 •

edited

Loading

KrisThielemans commented Nov 8, 2021 •

edited

Loading

VietAnhDao commented Nov 9, 2021 •

edited

Loading