SRT2D repository: SRT 2D PET and SPECT algorithms

Dimitra-Kyriakopoulou commented 1 month ago

Changes in this pull request

2 new analytic reconstruction algorithms SRT 2D PET in the src→ analytic → SRT2D folder and SRT 2D SPECT in the src→ analytic → SRT2DSPECT folder of my SRT2D repository. [In 2016 in SRT2D folder had been uploaded the prior versions of both these two codes: these old versions do not work with the current STIR version (as there had been pointer etc changes in STIR in between). Also the new versions of the 2 algorithms are improved, e.g. the old SRT2DSPECT run only for uniform data, whereas the new one doesn’t have such restriction.]

Testing performed

I installed STIR's last version of 7 February 2024 on my pc, and run the codes: they compiled, and they gave correct results. SRT2D run with the Hoffmann phantom, and SRT2DSPECT with XCAT phantom. As SRT mathematically works for the open disc, the slices of XCAT that touched the scanner were slightly squeezed at the edge (by interpolation).
The codes had no problem with the arc-correction functionality in terms of successful compilation; however, I did not run them with non-arccorrected data.

Related issues

Checklist before requesting a review

[] I have performed a self-review of my code
[] I have added docstrings/doxygen in line with the guidance in the developer guide
[] I have implemented unit tests that cover any new or modified functionality (if applicable)
[] The code builds and runs on my machine
[] documentation/release_XXX.md has been updated with any functionality change (if applicable)

KrisThielemans commented 1 month ago

@Dimitra-Kyriakopoulou please don't update pre-commit-check.yml. If you cannot run clang-format on your files (which is trickier than it should be), I can run it for you. Don't worry about that.

Dimitra-Kyriakopoulou commented 1 month ago

Thank you so much for your message! I am sorry for committing continuously.

Now Pre-Commit-check passed; it seems it had a problem not with clang-format, but because Node.js 20 instead of Node.js 16 should be used. Hence, in pre-commit-check.yml the line which was causing the problem was:

uses: pre-commit/action@v3.0.0

I replaced it with

- name: Setup Node.js 20.x # Ensuring Node.js 20 is set up uses: actions/setup-node@v3 with: node-version: '20.x' # Use Node.js 20.x

I hope this does not cause a problem in the following stage.

Codacy static and Continuous-integration again passed; however the latter does not appear in the pull request page, but it just finished running in STIR-AppVeyor.

Thank you so much for your help!!

KrisThielemans commented 1 month ago

@Dimitra-Kyriakopoulou can you please make multiple commits locally, and then push. Alternatively, add [ci skip] to a commit message. Every new commit triggers a job on Appveyor that takes ~90 mins run-time. I'm killing most of these...

Dimitra-Kyriakopoulou commented 1 month ago

I am really sorry, I forgot [ci skip] in my current change! Please cancel from AppVeyor.

Dimitra-Kyriakopoulou commented 1 month ago

Thank you wholeheartedly for your review!! I am really sorry with committing continuously; thank you for your help with [ci skip].

If the changes I applied are not what you asked for, may I please ask you please helped me with further comments?

-Unfortunately, pre-commit-check.yml reverted to its original content, failed again. The problem is the line: uses: pre-commit/action@v3.0.0 (which allows for use of Node 20 instead of Node 16, and maybe this transition does not occur in my repository. Let me also note I have 18.04.01 Ubuntu, not the latest.)

-Each review point now has a sign 'Outdated'. Am I to click on the 'Resolve Conversation' button? I would not press this button neither for the pre-commit-check.yml nor for the headers (as I am not certain if the new headers are fully OK).

-In the SPECT code, I had forgotten (and now erased) a line which allowed to choose the slices of a sinogram for a multi-slice phantom (as I was running XCAT); apparently this would lead to wrong results if run with arbitrary data.

Thank you wholeheartedly!! I am looking forward to your next comments!

Dimitra-Kyriakopoulou commented 1 month ago

I think what I wrote about the Nodes might not be the problem. It seems to me now it is indeed the Clang-format... I am please asking for your help on fixing it. Thank you so for all your help!!!

Dimitra-Kyriakopoulou commented 1 month ago

The option 'Re-request Review' appeared, and I clicked on it, thinking it was due to the previous review comments being labeled as 'Outdated' after the changes applied. I now think though if the pre-commit-check.yml should have first been corrected; hence, if pressing this button was wrong, please ignore it, and I am really sorry.

KrisThielemans commented 1 month ago

Below is a screen-shot of output of recon_test_pack/run_test_simulate_and_recon.sh with STIR compiled in Debug-mode, STIR_OPENMP=OFF. Top: SRT2D, bottom FBP2D. This looks fine except from an overall scale factor of 2. This will be because of the choice of image units. As the recon_test_pack runs with zoom:=0.5, SRT2DReconstruction needs to divide by zoom at the very end to get same units as the rest of STIR (for PET).

Dimitra-Kyriakopoulou commented 1 month ago

Dear Professor @KrisThielemans, THANK YOU WHOLEHEARTEDLY FOR ALL YOUR INVALUABLE HELP!!!

I am afraid I do not see the screenshot.

However, I think there is something else also going on leading to the failure of Build and ctest and recon_test_pack CI tests. As I need to attach some files to explain, I am writing email to send. THANK YOU WHOLEHEARTEDLY!!!

KrisThielemans commented 1 month ago

Release STIR_OPENMP=ON recons crash (also on GitHub Actions) however. This is a problem with the OpenMP code (I've tested Release, disabling the OpenMP pragma, and then it's fine).

@Dimitra-Kyriakopoulou wrote

I had run openmp in the past, not now. However, I had not run it with the choice dynamic. I changed it to dynamic, because continuous-integration asked me for dynamic allocation of variables, hence I thought it might also be best it is set so. I wonder whether this causes the openmp problem.

Not sure what you mean with "dynamic". Which lines in the code? I removed schedule(dynamic) nowait but that didn't help

Dimitra-Kyriakopoulou commented 1 month ago

Dear Professor @KrisThielemans, I would please like to inform you on the following: 1) I uploaded the new versions of the algorithms so that OPENMP runs. The SPECT version parallelization has artifacts, hence I need to improve it.

2) I could not divide by zoom for PET: the run_test_simulate_and_recon.sh appeared to substitute the zoom variable with a value of its own. This was happening even if there was NO division by zoom, but zoom was simply defined in the code; that's why I had to comment it out. However, locally on my pc, outside of recon_test_pack, using dividing by zoom, was possible, and changes in the default value, or in the par file, impacted the result accordingly. I had to leave multiplication by 2, so that the test would pass.

3) Regarding the SPECT test, I realized the following:

Because of the uniformity of the phantom, I thought I could just use the PET data with a SPECT scanner, but I then realized this would be wrong as the center of the phantom is shifted and does not coincide with the center of the coordinates.
Also I initially thought 'precorrected' and 'acfs' would correspond to arc-corrected and the attenuation sinogram, resp. ... When I read the code I realized this was wrong.
Hence, to have the test, inside run_test_simulate_and_recon.sh, I took the images of the phantom and the attenuation, and tried to forward project them with STIR's SPECT scanner. I made them compatible, but in a wrong way, as the result of the reconstruction was wrong...
My thought on the test is that creating the correct sinograms (SRT takes as input not the attenuation, but its sinogram), the same test as in run_test_simulate_and_recon.sh could be used. However, there is an EXTRA problem:
SRT2DSPECT was created mathematically for the open disc. I noticed that whatever the impact of zoom in the test is, it causes some slices to touch the scanner, hence SRT fails there. To deal with this I tried to include in the code linear interpolation to squeeze the input data, but this had bad effect on the quality of the result for the slices that did not touch the scanner...

I will be waiting to see the results of the tests, esp. if the repository runs with OPENMP, and if PET passes run_test_simulate_and_recon.sh, as it happens locally on my pc. Then I will be waiting for you guidelines on what should happen with the SPECT test. Could it be that a separate .sh test should be created (to avoid the impact of zoom)?

THANK YOU WHOLEHEARTEDLY FOR ALL YOUR INVALUABLE HELP!!!

Dimitra-Kyriakopoulou commented 1 day ago

Dear Professor @KrisThielemans, I created the tests for SPECT changing 3 reckon test files. Attempting to change the name of simulate_PET_data_for_tests.sh to the now appropriate simulate_PET_and_SPECT_data_for_tests.sh, a conflict was created, which would not allow Appveyor to run; so I rewrote the name as it initially was. Should Appveyor find errors, I will proceed to correct them.

THANK YOU WHOLEHEARTEDLY!

UCL / STIR