Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SRT2D repository: SRT 2D PET and SPECT algorithms #1420

Open
wants to merge 116 commits into
base: master
Choose a base branch
from

Conversation

Dimitra-Kyriakopoulou
Copy link

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)

@Dimitra-Kyriakopoulou Dimitra-Kyriakopoulou marked this pull request as draft May 10, 2024 22:11
Corrected the 2 mistakes in the Codacy report
Correction of the 8 (potentially minor) mistakes of the Codacy report
…e_PET_data_for_tests.sh [ci skip]

Applied Codacy static requested changes
Also changed the file's name to the original, because it created a conflict which prevented Appveyor from running
Applied changed requested from Codacy static
@Dimitra-Kyriakopoulou
Copy link
Author

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!

Checking if this change can lead to running  
Build and ctest and recon_test_pack CI
Checking if this change will run the Build and ctest and recon_test_pack CI tests
Reverting the last change to the original file, as the change did not lead to the running of Build and ctest and recon_test_pack CI tests
There is no chage in the file. As only Codacy and not CI test runs after changes in build-test.yml, I had to make a space change here for Appveyor to run for my pull-request after my last build-test.yml change
@Dimitra-Kyriakopoulou
Copy link
Author

Dear Professor @KrisThielemans,
I am afraid I do not know how to run the Build and ctest and recon_test_pack CI tests. Also the Run pre-commit check had run, but does not appear here.
May I please ask if it would be possible you applied these tests; so that if there are issues I could do the further work necessary for this commit to be eligible for merging.

THANK YOU WHOLEHEARTEDLY!!!

I had forgoten the last line: 'fi'
@Dimitra-Kyriakopoulou
Copy link
Author

Dear Professor @KrisThielemans,
I am afraid that in the file simulate_PET_data_for_tests.sh I forgot to include the last line, which would be 'fi'!
I did not upload the file, but copied it, and somehow missed selecting the last line... I am really sorry for that.
Thank you so much for the tests!!!

Also I have the impression that maybe this file could be renamed to 'simulate_PET_and_SPECT_data_for_tests.sh'; I could not rename it because this led to a problem in merging.

THANK YOU WHOLEHEARTEDLY FOR ALL YOUR INVALUABLE HELP!!!

I changed a whitespace, so that Appveyor would run
@Dimitra-Kyriakopoulou
Copy link
Author

Dear Professor @KrisThielemans,
I am afraid I had forgotten to upload the forward_project.par file used in simulate_data.sh !
I suspect this is the reason the 4 tests failed. However, it would be good in case it would be possible you sent me the my_create_SPECT_sino_SPECT.log.
I will now upload the missing file. I am deeply sorry for this 2nd careless mistake of mine.

THANK YOU WHOLEHEARTEDLY FOR ALL YOUR INVALUABLE HELP!!!

corrected name of file, so that it does not include the term 'SPECT' twice
@Dimitra-Kyriakopoulou
Copy link
Author

Dimitra-Kyriakopoulou commented Jul 16, 2024

my_create_sino_SPECT.log
Screenshot from 2024-07-16 13-55-54
Screenshot from 2024-07-16 13-56-20

Dear Professor,
THANK YOU SO MUCH FOR RUNNING THE TESTS AGAIN!!!

Locally on my pc the tests run; I attach the screenshots. Also the reconstructed image is ok.

I) The Build CI error is that forward_project fails. Could you maybe access the my_create_sino_SPECT.log file that was created, in case it has extra info than my .log file?

II) Locally on my pc my_create_sino_SPECT.log, which I attach contains 5 warnings, which I can't imagine causing this fail.

  1. WARNING: RadioNuclideDB::get_radionuclide: unknown modality. Returning "unknown" radionuclide.
  2. WARNING: SPECTUB matrix can currently only use single-threaded code unless all views are kept. Setting num_threads to 1
  3. WARNING: Using OpenMP with number of threads=1 produces parallel overhead. You should compile without OPENMP support
  4. WARNING: Image radius (166.500015 is larger than max detector radius (166.500000). Are you sure this is correct? (Proceeding anyway)
  5. WARNING: forward_project. Imaging modality unknown for either the image or the projection data or both. Going ahead anyway.
  • I will change the 4th.

  • Regarding the 5th, what I had to do was to comment the 'modality' name from phantom and attenuation images, as SPECTUB has modality 'nucmed', and there was conflict.
    It would be easy to replace for the SPECT case 'modality' to nummed for phantom and attenuation images used.

III) For run_test_simulate_and_reckon.sh to run the emission and attenuation data cannot be altered; only _the SPECT scanner parameters can be altered, and of course the forward_project.par file.
Could you please help me understand and apply the needed changes to the files with which forward_project is called, so that it will run with the Build CI tests as well? The my_create_sino_SPECT.log file that Build CI test created most likely will be critical; do you think it could be possible you might please accessed it?

I am really sorry for all the trouble. I am looking forward to your reply. THANK YOU WHOLEHEARTEDLY FOR YOUR PATIENCE, AND ALL YOUR TRULY INVALUABLE HELP!!!

@Dimitra-Kyriakopoulou
Copy link
Author

Dear Professor @KrisThielemans ,
I am deeply sorry I just found another careless mistake of mine: the SPECT_test_Interfile_header.hs had
name of data file := SPECT_Interfile_header.s
instead of
name of data file := SPECT_test_Interfile_header.s

Locally in my reckon_test_pack I had both SPECT_test_Interfile_header.hs and SPECT_Interfile_header.hs (because I wanted to have a backup in case of changes), hence I apparently copied the content of the 2nd, while having given the name of the 1st...

I now corrected the mistake, and Appveyor and Codacy run again.

I am really sorry. I hope this is the last mistake...

THANK YOU WHOLEHEARTEDLY FOR YOUR PATIENCE, AND FOR ALL YOUR INVALUABLE HELP!!!

@KrisThielemans KrisThielemans self-assigned this Jul 16, 2024
@Dimitra-Kyriakopoulou
Copy link
Author

Dimitra-Kyriakopoulou commented Jul 17, 2024

Dear Professor @KrisThielemans ,
THANK YOU WHOLEHEARTEDLY FOR RUNNING THE TESTS AGAIN!!!

I need to change a typo I noticed in the error message for SPECT sinogram.

I wonder whether the file simulate_PET_data_for_tests.sh might be changed to
simulate_PET_and_SPECT_data_for_tests.sh.
I had tried that in the past, but had conflict; hence, should you approve of this change, you can only apply it.

THANK YOU WHOLEHEARTEDLY FOR YOUR PATIENCE, AND ALL YOUR INVALUABLE HELP!!!

@Dimitra-Kyriakopoulou
Copy link
Author

Dear Professor @KrisThielemans ,
I also think the line
info(boost::format("View %d of %d") % ith % sth);
which I had in SPECT, so that the user knows the program is not stuck,
should be removed: it also delays running a bit.

THANK YOU WHOLEHEARTEDLY FOR YOUR PATIENCE, AND ALL YOUR INVALUABLE HELP!!!

correction of typo in error message
removed terminal message
@Dimitra-Kyriakopoulou
Copy link
Author

Dear Professor @KrisThielemans ,
The 9 tests were successful ! But I made the changes I stated above, and as a result I am afraid the Build reckon CI tests will need to rerun ... I am sorry for the trouble

THANK YOU WHOLEHEARTEDLY FOR YOUR PATIENCE, AND ALL YOUR INVALUABLE HELP!!!

@KrisThielemans
Copy link
Collaborator

Thanks @Dimitra-Kyriakopoulou, success!

I will provide a review later, but probably only second half of August due to commitments. In any case, we know this is good to go now!

@Dimitra-Kyriakopoulou
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants