ofgulban / mp2ragelib

MP2RAGE functions implemented in the way I understand better :)
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

Typo for TC values in compute_T1_lookup_table() + definitions of TA/TB/TC based on Partial Fourier #1

Open mafortin opened 3 months ago

mafortin commented 3 months ago

Hello :)

First, nice and simple yet efficient repository! This repository is exactly what I was looking for: an user-friendly and simplistic pythonic implementation of the MP2RAGE T1 mapping technique. If someone would think small personal GitHub repositories like this one might be useless, now you have the proof that, even 4 years later, people are still using your work. :)

Regarding the issue, I believe that there is a typo at line ~235 of core.py in the function _compute_T1_lookuptable() where TC is defined. It should be _TC = TR_MP2RAGE - (TI_2 + (NR_RF TRGRE / 2)) but _TC = TR_MP2RAGE - (TI_1 + (NR_RF TRGRE / 2)) is written.

Moreover, this might not be an issue per se depending if Partial Fourier was applied or not, but the variable _NRRF might have a different value than the one used in this script. Since the SlicePartialFourier parameter for MP2RAGE impacts the 'inner loop' (i.e., on the small flip angles and short TRs in each RAGE block, and not the 'outer loop' (i.e., long TRs)), this results in a different time when the center of k-space is reached than _(n_gre TRgre)/2. Since you want to reach the center of k-space as early as possible, the PF factor is applied at the beginning of the RAGE block resulting in reaching the k-spoace center at _t < (n_gre TRgre)/2. E.g., if PF=6/8 and _NRRF=200 --> _NRRFeff = 150 where the number of RF pulses before and after the k-space center go like this: _NR_RFbef = 50 and _NR_RFaf = 100.

I am writing this in case it wasn't taken into account, but you have probably considered this in your own implementation and the latter comment can be ignored. :)

Thanks again for your work, greatly appreciated! :)

Marc

ofgulban commented 2 months ago

Hello @mafortin ,

Thanks a lot for your kind words and thanks for rising this issue. Actually, I happened to stopped working on this MP2RAGE library because we ended up using another sequence for our purposes. I would be more than happy if you would send a pull request to implement your suggested changes :)

I am very happy to hear that this mini project ended up helping you years later 😄 . Sorry for the late reply, I have been away from work for a while after the busy conference season. But your message made my day back at work 😃

Very best wishes, Faruk

mafortin commented 1 month ago

Hi @ofgulban ! :)

I first started to modify your repository to realize that there were several things I wanted to do differently (not related to the typos or quality of the work, but mostly personal preferences for code structure and scripts :) ), so I ended up creating my own repository which is basically my own implementation of yours (with explicit credits given to you). It should be public and available with that link: https://github.com/mafortin/mp2rage-py .

While I think I did fix the typos in my own implementation, I faced some basic programming issues. Simultaneously, I had to switch focus on other tasks at work which resulted in my implementation being not that useful in its current state since it can't compute accurate T1 maps haha!

Thanks for your answer and have a great day! :)

Marc

ofgulban commented 1 month ago

An I see, thanks for the information @mafortin . Then I think I will keep this issue open and when/if I came back to this, I am going to consider your suggestions :)

It is great to see that this work being used somehow, good luck with your implementation & work! And thanks again for being so kind to open this issue :)

Have great day as well!

Faruk