-
Notifications
You must be signed in to change notification settings - Fork 21
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
update Gradient descent notebook #218
Conversation
KrisThielemans
commented
Mar 18, 2024
- use minus log-likelihood for PET and fix wrong comments about ascent/descent for MR and PET
- fix has_astra => have_astra
- no longer use relative step-size as it fails for MR and CT
- hide PET warnings
@DANAJK I'm currently still using the "simulated" data. This has the disadvantage that a gradient descent step immediatelly gets the correct solution (but then things with the current default step-size get into difficulties). With your new data, I also have to set the tau smaller (.1 as opposed to .3), and then get I'm tempted to leave this as-is for now as updating the data will be hard (including retagging SIRF) |
My new data was a Shepp-Logan phantom so not sure how you get a real looking image! |
😄 ok. I must have run this with the "simulated" data (from brainweb), as in the notebook. I just now tried to reproduce that on a GitHub Codespace, but I get garbage backprojection (and reconstruction...) Maybe you could give it a go? image = bwd_mr.as_array()
centre_slice = image.shape[0]//2
plt.figure();plot_2d_image([1,1,1],numpy.abs(image[centre_slice,:,:]),'bla', cmap="viridis") I'm a bit lost in different versions that I have of everything at the moment. |
correction: the |
Yes, we get the parameters for the acquisition model from the .h5 file. We also use the .h5 file to calculate realistic coil maps. Unfortunately, they need to somehow match the brainweb data. This I guess is the reason for the strange image which you had before which looked like it had bits cut out. These were the black ellipses in the centre of the Shepp-Logan-phantom which don't have any signal. This lead to 0 in the csm due to some threshholding which we had at some point. Probably by solving this issue #1221 we indirectly fixed this notebook in 3.6 |
interesting. Thanks for that clarification, @ckolbPTB. So, this PR is good to merge? I might take the opportunity to move it to Introduction, as I suggested in #193 (comment) |
88dd153
to
3dfc9d3
Compare
- use minus log-likelihood for PET and fix wrong comments about ascent/descent for MR and PET - fix has_astra => have_astra - no longer use relative step-size as it fails for MR and CT - hide PET warnings
3dfc9d3
to
e42d18d
Compare
0616955
to
6c2f6e8
Compare