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 fixes #1

Merged

Conversation

KrisThielemans
Copy link
Collaborator

  • I ran clang-format 14.0 (committed it as @Dimitra-Kyriakopoulou, to avoid lines being attributed to me). (diff looks large, but use "ignore whitespace" in the diff setting to see what not much changed)
  • I added a test in the recon_test_pack for SRT2D.

Running the test causes a segmentation fault however. Array indices are going out-of-bound. I found one place where this was due to arc-correction changing the num_tangential_poss. There seems to be another crash though.

@KrisThielemans
Copy link
Collaborator Author

Note that GitHub Actions isn't running on this PR apparently. It would when you'd merge it. However, possibly wait until tomorrow as I'm running this in a debug session overnight, so I might be able to tell you where the crash is.

@Dimitra-Kyriakopoulou
Copy link
Owner

Thank you wholeheartedly!!!
Would it may be possible that you please gave me the sinogram for which the code does not run? I believe this would critically help me.
Thank you wholeheartedly for all your invaluable help!!!

@KrisThielemans
Copy link
Collaborator Author

Actually, this runs through in debug mode with STIR_OPENMP=OFF. Image is fine! Great!

I suspect it is an OpenMP problem, but your implementation is a bit hard as you use some shared and some private variables, all declared upfront, before entering the loop. (It is far easier to declare most variables only where you need them in the loop, certainly loop variables such as for (int ia=...), as those will automatically be thread-private).But it could be something else...

I suggest you merge this, and then pull it to your machine and build and

cd recon_test_pack
./run_test_simulate_and_recon.sh

It will simulate the sinogram and then run SRT2D on it. Once you have run it once, you can do

export suffix=_force_zero_view_offset
SRT2D SRT2D_test_sim.par

@Dimitra-Kyriakopoulou Dimitra-Kyriakopoulou merged commit 23e8a5a into Dimitra-Kyriakopoulou:SRT2D May 20, 2024
1 check passed
@Dimitra-Kyriakopoulou
Copy link
Owner

THANK YOU WHOLEHEARTEDLY!!!

  1. I did the recon-test-pack test, as you told me. It produced result, which I sent my email, as it cannot be attached here.
  2. In the test there was the message
    Input ROI mean: 1
    Output ROI mean: 0.305791

However, after running
export suffix=_force_zero_view_offset
SRT2D SRT2D_test_sim.par

opening the created attached images in amide the value is approximately 0.8; this might be OK considering SRT has too much noise (without filter).

  1. 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.

THANK YOU WHOLEHEARTEDLY!!!

@Dimitra-Kyriakopoulou
Copy link
Owner

Hello! I found out why

Input ROI mean: 1
Output ROI mean: 0.305791

As I cannot attach the files, I will send email.
THANK YOU WHOLEHEARTEDLY!!!

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