Repetitiontime #13
Repetitiontime #13
Conversation
Add RepetitionTimePreparation
- Get rid of line breaks and white spaces - Remove diff w.r.t. previous version
- Wrapping long lines
- Remove residual +
There was a problem hiding this comment.
Thank you @agahkarakuzu!!
I have a question for discussion tomorrow about where to put all the explanation of the difference between RepetitionTimeExcitation and RepetitionTimePreparation. My gut would be to have the bullet points be shorter and then have a paragraph explaining the logic below, but Iets just see what everyone on the call would prefer as this is definitely not something I'm terribly bothered about!
bep001-naming-conventions.md
Outdated
| |---------|------------------------------------------|--------------------------------| | ||
| | VFA | Variable Flip Angle | T1Map | | ||
| | IRT1 | Inversion Recovery | T1Map | | ||
| | MP2RAGE | Magnetization Prepared 2 Gradient Echoes | T1Map, UNIT1 | |
There was a problem hiding this comment.
since you already have MP2RAGE here, how about its more generic form MPXRAGE? or could you use the more generic name now, and the json would indicate there were 2 echos.
There was a problem hiding this comment.
I can imagine MP2RAGE being the popular use case . From this perspective, keeping it as is seems reasonable. On the other hand, your suggestion would make this label semantically more accurate for multi-echo acquisitions and as you noted the rest can be revealed by the jsons. I don't have a strong opinion about this, can be briefly discussed in the meeting I guess @KirstieJane ?
bep001-naming-conventions.md
Outdated
|
|
||
| | Suffix | Method Name | Output Name | | ||
| |---------|------------------------------------------|--------------------------------| | ||
| | VFA | Variable Flip Angle | T1Map | |
There was a problem hiding this comment.
this is akin to using FLASH and can give rise to T1Map, T2*Map, and PDMap
There was a problem hiding this comment.
This was the reason why instead of using sequence names, method names were suggested in the suffix. For example, T2* mapping with multi-echo FLASH for can be grouped under another label, which would make it easier to signify the purpose of that collection of images. IMO, overloading one label with more than one method is not really helpful for users and validation.
| * `RepetitionTimeExcitation`: The time in seconds between successive excitation pulses that excite the same tissue. | ||
| As with `RepetitionTimeInversion` this corresponds to [DICOM Tag 0018, 0080](http://dicomlookup.com/lookup.asp?sw=Tnumber&q=(0018,0080)): "the period of time … between the beginning of a pulse sequence and the beginning of the succeeding (essentially identical) pulse sequence". | ||
| Note that although this would typically be called `RepetitionTime` please use `RepetitionTimeExcitation` for structural scans with multiple excitations as `RepetitionTime` is already defined as the amount of time that it takes to acquire a single volume in section 8.3.3 and to distinguish it from `RepetitionTimeInversion`. | ||
| * `RepetitionTimeExcitation`: The time in seconds between successive excitation pulses that excite the same tissue. The DICOM tag best refers to this parameter is [(0018, 0080)](http://dicomlookup.com/lookup.asp?sw=Tnumber&q=(0018,0080)): "the period of time … between the beginning of a pulse sequence and the beginning of the succeeding (essentially identical) pulse sequence". This may be superseded by `RepetitionTimePreparation` for certain use cases, such as [MP2RAGE](https://infoscience.epfl.ch/record/172927/files/mp2rage.pdf). Please see the description of `RepetitionTimePreparation` parameter for further information. Finally, please note that `RepetitionTimeExcitation` (0018,0080) is also widely refered to as `RepetitionTime` in MRI physics literature. However, `RepetitionTime` is already defined as the amount of time that it takes to acquire a single volume in section 8.3.3. Therefore, please use `RepetitionTimeExcitation` (along with `RepetitionTimePreparation`, if needed) for anatomy imaging data. |
There was a problem hiding this comment.
since you are defining this as a new key, would it make sense to keep the units the same as DICOM in ms?
There was a problem hiding this comment.
As other timing parameters in the main spec are described in seconds, I would say we keep it in seconds here for the sake of consistency.
There was a problem hiding this comment.
Yep 👍 on @agahkarakuzu's comment here. Here's the reference to the section in BIDS that says we have to convert to s from ms: https://github.com/bids-standard/bids-specification/blob/master/src/02-common-principles.md#units
There was a problem hiding this comment.
@KirstieJane, @agahkarakuzu - it was a suggestions since this PR is already deviating from the spec with respect to things that needed to be added.
do note the wording says SHOULD not MUST (even though it later says needs to), which means there is a weighing in place to be considered.
For certain things when the scale is in micro or nano secs, i don't think it makes sense to convert to seconds. BIDS being a text-based standard vs a binary standard actually results in representing float as text, which has all kinds of issues. representing numbers as integers when possible has several benefits. thus far with the focus on MR, and more specifically fMRI, most units are in s. As this and other BEPs from other modalities get merged, the units of consideration need to be weighed.
The specific DICOM parameter the section is referring to is RepetitionTime and the decision at that time was taken with fMRI in mind (80% use case), where the typical natural order is in seconds. the original DICOM standard was not created with fMRI in mind. it was closer to the MR physics definition of excitation. Since this PR is an attempt to bring those pieces into play, it would be good to consider and perhaps note why choosing s over a natural scale was more appropriate.
There was a problem hiding this comment.
In any practical case I can think of, RepetitionTimeExcitation has to be a float value. If you specified it in milliseconds, integers would not be sufficient. I frequently have TRs of e.g. 8.5ms. I think most would agree that microseconds are not a natural scale for anyone other than sequence programmers (I certainly do not want to have to write TR=8500 microseconds and then sprinkle 1.e-6 conversion factors through my code. I have to do that when programming in EPIC and it is painful). I have never needed nanosecond precision for MR. To my knowledge the finest time resolution on any existing scanner is 2 microseconds.
Hence in my opinion seconds are the natural scale for MR. Keeping the units in seconds massively reduces the number of conversion factors you have to add to equations, which is a common source of bugs (for me anyway).
There was a problem hiding this comment.
@spinicist - fair enough - the point of this discussion is to ensure that people do not choose a time unit just because, but because it is the appropriate thing to do so. it is critical that once the choice is made we do not change it, but let's ensure the choice is made with a wide range of use cases in mind. this discussion should be had not just for this particular key, for any key that is being introduced in this bep, and in the spec more generally. the intent of the SHOULD is that this is recommended, but you (the "royal you" creators of a new field) should still decide if it is appropriate.
regarding conversions people have to either change existing code or convert from BIDS units one way or the other. so that is less of a concern. people should be careful in their code (just ask NASA about their rover).
There was a problem hiding this comment.
Hi @satra & @spinicist! So sorry for my slow reply.
I like @spinicist's point about not wanting to write 1.e-6 everywhere. I agree that the repetition times are likely to be most humanly readable in seconds.
I had implicitly interpreted the SHOULD slightly differently to @satra - I had read it as "unless you have a good reason not to (for example if we were including values in years!). This is different to the idea that the royal "you" 👑 needs to make an informed decision every time.
My position is that we have had the conversation. We've decided to keep seconds. And I propose that we do not include the justification in the documentation anywhere.
So specifically, can I mark this conversation as resolved? Do we need to add anything to the specification?
There was a problem hiding this comment.
@KirstieJane - a lot of bids came out of the cognitive neuroimaging world where fMRI rules supreme. bep001 comes out of tissue quantification world, and in that world many of the sequences are more tuned to other things, and we will continue to see changes. in this world, times are represented typically in different units:
https://journals.plos.org/plosone/article?id=10.1371/journal.pone.0169265
i'm fine with marking this conversation as resolved, but i do want people to consider things beyond a few years or a few sequences. my point was to ensure that people think through before setting in stone, which is often what a standard is. some things are much harder to change than others, and thus require more careful consideration.
|
This pull request has been superseded by #24 |
Suggestion: Replace
RepetitionTimeInversionwithRepetitionTimePreparationto have a more flexible parameter that can also be used for sequences with preparation blocks other than a single inversion pulse.The definition of
RepetitionTimeExcitationremains as suggested in the original proposal by @KirstieJane. Some edits are made on the description to comply with changes regardingRepetitionTimeInversionand for clarity.Many thanks to @mathieuboudreau and @spinicist for their valuable contributions.