-
Notifications
You must be signed in to change notification settings - Fork 191
[ENH] Add DWI recommended metadata #2309
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
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2309 +/- ##
=======================================
Coverage 82.81% 82.81%
=======================================
Files 22 22
Lines 1693 1693
=======================================
Hits 1402 1402
Misses 291 291 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "DiffGradientDuration": 11.0, | ||
| "DiffGradientSeparation": 15.2, |
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.
We don't generally use abbreviations. I would say either "DiffusionGradientDuration/Separation" or "GradientDuration/Separation". I would probably prefer the latter if the terms are unambiguous within a diffusion context and wouldn't refer to something else in another context BIDS is likely to touch on.
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.
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.
I don't necessarily like the word "separation", which to me implies the interval between the end of one pulse and the beginning of the other pulse. Other words to potentially use here are "interval", or "offset". I do wonder if given their prevalence in the literature, BigDelta and SmallDelta wouldn't be clearer. Also, I do believe that this is a redefinition of something that could already be captured by MixingTime: https://bids-specification.readthedocs.io/en/stable/glossary.html#mixingtime-metadata?
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.
I see this as related to #2311
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.
I'm not familiar with how diffusion sensitization plays out in a "double spin echo" diffusion weighted sequence, but are MixingTime and BigDelta really the same thing?
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.
The timing parameters are defined quite differently for "double" or "twice refocused" spin echo sequences, see https://onlinelibrary.wiley.com/doi/10.1002/mrm.10308. IIUC (see Fig 1 therein), there is no "BigDelta" in these sequences, and the intensity of the gradients is somehow integrated across all those small deltas, so I think that's to be handled separately, maybe in the advanced dMRI BEP.
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.
So then, the existing MixingTime is not appropriate here, right?
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.
I am not too familiar with MixingTime but this paper declares the mixing time, gradient separation, and gradient duration as separate values:
...mixing time TM=173.0 ms, gradient separation Δ=185.8 ms and a gradient duration of δ=5.4ms.
|
I believe that some large datasets have been using their own versions of this (e.g., HBCD has LargeDelta and SmallDelta), so it would be good to get input on the naming from some folks who might be doing something similar (@mharms, @Lestropie, @arokem). |
|
What about sequences that might allow for differing deltas across the differing volumes? How would that be handled? |
|
Also, are BIDS timing parameters no longer all specified in seconds? (This proposal was specifying these parameters in msec). |
That's exactly our use case for our imminent data release. There are 2 competing options right now: separate text files like bvec and bval, or a sequence of numbers in the JSON file. Would love to get a consensus that involves bvec, bval, TE, δ, Δ all being handled the same way. |
Well, the treatment of bvec and bval as separate text files is longstanding, and that presumably isn't going to change. Does the current standard support an array for TE, if the TE is changing as well? |
We have 42 fields with units There are two ASL metadata fields and one MRS field that use milliseconds ( I think it does make sense to stick with seconds, but it's not unprecedented to use ms. |
Yes, TE does allow for an array of numbers. Please see Timing parameters. |
Happy to switch to seconds if that is common across BIDS. I opted for milliseconds here since that is how δ and Δ (and TE) are reported in manuscripts. |
@effigies: |
|
It's like with |
|
I can start a separate PR for this if needed. |
|
No, that would break backwards compatibility. Tools could probably figure out the scale by looking at the value, but it's not worth having one interpretation in one version and another in another. |
|
Hi all, thanks for the feedback. I have updated this pull request. Next I will update #2320. |
| "BigDelta": 15.2, | ||
| "SmallDelta": 11.0, |
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.
| "BigDelta": 15.2, | |
| "SmallDelta": 11.0, | |
| "BigDelta": 0.040, | |
| "SmallDelta": 0.015, |
Shouldn't the example have something reflective of seconds?
| format: participant_relative | ||
| BigDelta: | ||
| name: BigDelta | ||
| display_name: Diffusion Gradient Separation |
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.
I'm not sure where/how these "display_name" fields show up, but I wonder if it should just be "Diffusion BigDelta"?
cc @satra @ayendiki @Tinggong