search

RTKConsortium / RTK

Reconstruction Toolkit
Apache License 2.0
244 stars 144 forks source link

BUG: Fix the inconsistency between CUDA and multi-threads(MT) code for negative SourceToDetectorDistance #532

Closed theysp closed 1 year ago

theysp commented 1 year ago

I have found that for geometry that have negative sdd and sid, which means the projection geometry is described in a right handed coordinate system, the DisplacedDetectorFilter implemented on CUDA behaves differently, and the CUDA version obviously takes the wrong direction of the detector row and gives wrong weights on the padded projections. After looking into the implementation, I have found that the function ToUntiltedCoordinateAtIsocenter has been implemented differently in CUDA and MT versions. I have altered function ToUntiltedCoordinateAtIsocenter in rtkCudaDisplacedDetector.cu, to make it have consistent results with rtkDisplacedDetector.hxx.

theysp commented 1 year ago

It seems the RTK is fully tested with left handed geometry, which makes the reconstructed images having opposite Z direction with the scanned objects. But right handed geometry is more nature. This issue makes me confused all the time. Do you know why left handed geometry and clock-wise rotation angle being used in a lot of CBCT systems?@SimonRit

SimonRit commented 1 year ago

Thanks for your contribution. Can you please update the commit message to follow ITK guidelines?

The inconsistency has been introduced by ad58d3c110b98254e9ff33e4de83697e852baaf6. Maybe other Cuda filters need to be updated too. I would appreciate it if you have the time to check.

I'm not sure I understand what you mean by left handed... but I usually have positive source-to-isocenter and source-to-detector distances with the systems I deal with.

theysp commented 1 year ago

Thank you for the clue. I have checked the revisions you've done in ad58d3c. This commit is OK with consistent between the CUDA and non-CUDA implementations.

The inconsistency happens between the rtkDisplacedDetectorImageFilter and rtkCudaDisplacedDetectorImageFilter. I think this error exists because displaced detector images is not used a lot with negative SDD as you said.

Thanks for your contribution. Can you please update the commit message to follow ITK guidelines?

The inconsistency has been introduced by ad58d3c. Maybe other Cuda filters need to be updated too. I would appreciate it if you have the time to check.

I'm not sure I understand what you mean by left handed... but I usually have positive source-to-isocenter and source-to-detector distances with the systems I deal with.

SimonRit commented 1 year ago

I see you've updated the PR title, can you amend the commit message instead? The prefix BUG: is good but the message should not be longer than 72 characters, maybe BUG: Fix CUDA/CPU inconsistency in DisplacedDetectorImageFilter Then you can explain the negative "distance" in the message body. If you don't mind, can you also add a line in rtkdisplaceddetectorcompcudatest.cxx to test a negative SID? Thanks again!

theysp commented 1 year ago

I see you've updated the PR title, can you amend the commit message instead? The prefix BUG: is good but the message should not be longer than 72 characters, maybe BUG: Fix CUDA/CPU inconsistency in DisplacedDetectorImageFilter Then you can explain the negative "distance" in the message body. If you don't mind, can you also add a line in rtkdisplaceddetectorcompcudatest.cxx to test a negative SID? Thanks again!

Currently I am in a environment that's a little difficult to have some test.. I will add a new line in rtkdisplaceddetectorcompcudatest.cxx to test a negative SID later in a week.