-
Notifications
You must be signed in to change notification settings - Fork 161
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
[ENH] MRI: Improve definition of bvec file #1811
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1811 +/- ##
=======================================
Coverage 88.22% 88.22%
=======================================
Files 16 16
Lines 1393 1393
=======================================
Hits 1229 1229
Misses 164 164 ☔ View full report in Codecov by Sentry. |
Looks good to me. I might suggest a very minor rewording, but it might be more a matter of personal preference than clarity... This sentence:
Modify to:
In fact, I'd be tempted to mention this particular aspect before describing the interpretation as relative to the image axes, to ensure that there is no confusion about when the sign flip takes place: it applies to the original entry in the bvec table, not the x-component following transformation into scanner space or something. I would also have a more upfront statement about the fact that bvecs are relative to some reference image, maybe in the second sentence, with something like:
As it stands, the fact that the reference image must be supplied is implicit from the later discussion, but in my opinion it's a really important consideration that deserves to be stated very upfront. |
Maybe Mark Jenkinson, @markj789? |
Had a go at some minor tweaks. The directions being defined with respect to the image axes is stated explicitly twice, and the ordering of statements about flipping of first element / contrast against DICOM convention is hopefully better. |
Marked as ready for review. |
for more information, see https://pre-commit.ci
Co-authored-by: Christopher J. Markiewicz <[email protected]>
b413ca7
to
61be6bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattcieslak and I discussed this and it looks good to us.
Thanks @Lestropie for the PR, and thanks all for the reviews! |
Closes #243.
Making an attempt to resolve this issue as ensuring that the definition here is robust and consistent across different softwares will be essential to verifying the definition of orientation encoding for BEP016 MRI diffusion models.
References:
I'm listing this as a draft PR as it's entirely possible that the phrasing can be fine-tuned. Pinging @jdtournier; if anyone knows of an appropriate person from FMRIB to ping please do so.
One note here is that I've intentionally avoided the use of "radiological" vs. "neurological" coordinate systems. I've always thought of these as being in reference to two-dimensional projections of image data. There seems to be some confusion about which of these terms apply in reference to three-dimensional coordinate systems. Therefore I chose to stick to "left-handed vs. right-handed" / "positive vs. negative determinant". Not strictly against use of such terms, but the ambiguity would need to be resolved.