[ENH] Add coil entity for uncombined MR data #425
Conversation
There was a problem hiding this comment.
Thank you for the PR. I particularly found of it because it is in the vein of #371, so should be easier to review/accept than larger full blown PRs.
But as you mentioned in #370 and #370 (comment) in particular, there is a bit of "competition" between using coil and channel terms. On one hand, I sway with BEP004 (attn @bids-standard/bep_leads) in using _coil since that is what BEP004 already uses, and to say the truth - despite ambiguity I used term coil more than channel as to describe individual "antennas" ;). But indeed it might not only be a bit "confused" with "coil" as the term for overall transmit and/or receive coil, but what is probably more important is that, in principle, MRI (please correct me if I am wrong) cannot receive data from coil directly!
Data arrives from channels with their amplifiers/filters etc. It is typical (again -- correct me if I am wrong) that each coil is connected to some channel in one-to-one mapping, thus giving us this headache of deciding coil vs channel . But it might not be the case in case of "phased arrays". Also in case of "switchable arrays", it would be different coils (physically) connected to the same channels depending on acquisition. So it feels that storing channel should be more adequate. Also in BEP004, coil is used with an index (01, 02, 03, ...) thus not even taking advantage of available coil names etc (placing CoilName into .json is not yet suggested). So effectively it is merely a "disambiguation" index and not necessarily referring to a specific coil.
So, altogether, I would prefer channel over coil, but question is -- which field in DICOM (if any) contains Channel Index of a kind, so having _ch-INDEX could be not only for disambiguation, but to reflect actual data acquisition detail?
But also, I think that this PR should be "blessed" (in one shape or another) by BEP004 in that it would be what they would use for SWI images, since that is where channel/coil is having direct applicability.
I have also left some feedback on wording etc requesting changes.
Cheers!
| data from sequences not employing coil combination. | ||
| The key `CoilString` MAY also be added in the JSON file, with a corresponding | ||
| identifier for the channel within the coil. | ||
| If `ch-<index>` is not used, data are assumed to be combined across channels. |
There was a problem hiding this comment.
I would remove this statement. If you grep for "assume" you will see that it is not common to state assumptions in BIDS. Moreover, I could have individual channels and combined data.
There was a problem hiding this comment.
In the current proposal, they say "if skipped total is assumed", so my thinking was that having both channel-wise and combined data would look like:
func/
sub-XXX_task-YYY_bold.nii.gz # combined
sub-XXX_task-YYY_ch-01_bold.nii.gz
...
sub-XXX_task-YYY_ch-32_bold.nii.gz
First, do you think it makes sense to mix having the entity and not within the same run like that or should the specification allow key/value pairs with an index OR a label (specifically, "combined")?
Second, would a better way of saying that there's a default be like the following (from the run entity description):
When there is only one scan of a given type the run key MAY be omitted.
So maybe:
When only combined data are reconstructed the channel key MAY be omitted.
There was a problem hiding this comment.
First, do you think it makes sense to mix having the entity and not within the same run like that or should the specification allow key/value pairs with an index OR a label (specifically, "combined")?
IMHO it is ok.
Second, would a better way of saying that there's a default be like the following (from the run entity description):
When there is only one scan of a given type the run key MAY be omitted.
So maybe:
When only combined data are reconstructed the channel key MAY be omitted.
run is a bit of a different beast IMHO to make direct analogy here. The suggested phrase sounds a bit off to me, since if data (in the file) is combined, there can be no channel. Overall, I would not overdo it -- the purpose of all those suffixes is to provide high level information about the file and to disambiguate from one file to another. So, may be we could flip it to
When file contains data from a single channel,
ch-<index>SHOULD be provided
Then, in the case when "combined" data is actually combined from a single channel recording (is that within even 0.1% of cases? ;)), it would be up to a user to decide to include or not the ch key. If they have multiple channels, the need for using _ch becomes obvious -- to disambiguate.
There was a problem hiding this comment.
Thank you! I've incorporated your suggestions.
There was a problem hiding this comment.
I am not sure whether it is the right place to post, but I would like to warn about confusing channel and coil. As we have just discussed with @tsalo on mattermost, channels refer to the two parts of the quadrature used by coils (see: http://mriquestions.com/real-v-imaginary.html) resulting in real and imaginary (or magnitude and phase) data.
Therefore, I suggest using coil rather than ch(annel) to refer to coils.
There was a problem hiding this comment.
I've switched from ch to coil. Thank you for the insight @tiborauer.
There was a problem hiding this comment.
@tiborauer do you have example of such data? wouldn't you have then 2 _ch- files - one representing I and another Q? sure thing then someone could transform them into "magnitude" and "phase" images, making it needing some _coil or another entity to represent "combination" of data from different channels? (so may be we need both _ch and _coil?)
There was a problem hiding this comment.
@yarikoptic, I am afraid I do not work with raw data (in MRI physics POV), but any data could be saved as such.
I agree with you that channels refer to the two parts: real and imaginary (see the link in my previous comment), which can be transformed into magnitude and phase; which means that entity part (referring to magnitude and phase and also part of BEP001) and ch (referring to real and imaginary) are different. Entity coil, however, should correspond to another level, namely the transmitter/receiver hardware usually containing multiple coil elements. See http://mriquestions.com/array-coils.html explaining Coil Elements > Segments > Channels; so you may need even more entities. :)
(N.B. You may also want to differentiate between transmitter and receiver coils because different coils can be used for excitation and reception.)
There was a problem hiding this comment.
As proposed in #508 (and #424 now), part supports both phase/magnitude and real/imaginary, so I'm not sure if that's still a concern.
I'm quite comfortable ignoring transmitter coils. I think it falls sufficiently outside the 80% BIDS is meant to cover. If someone is going to collect different runs with different transmitter coils, they can use acq to differentiate the files, I think.
There was a problem hiding this comment.
If you are happy with using the same entity part for phase/magnitude and real/imaginary, then it is fine. They are somewhat redundant anyway because one can compute one pair from the other pair. However, we have to be explicit about these - mutually exclusive - representations.
Distinguishing coils and channels is still a concern.
I agree with ignoring the transmitter/receiver coils. In most cases, the same coil(set) is used for both.
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
| The OPTIONAL `ch-<index>` key/value can be used to distinguish channel-specific | ||
| data from sequences not employing coil combination. | ||
| The key `CoilString` MAY also be added in the JSON file, with a corresponding | ||
| identifier for the channel within the coil. |
There was a problem hiding this comment.
is that really an identifier for the channel or for the coil that channel (currently) is connected? might want to adjust.
FWIW, verified that at least dcm2niix now would dump CoilString entry into the sidecar .json file for SWI file:
$> dcm2niix -b y -z y -o . MR.1.3.12.2.1107.5.2.43.167017.2020011616095414658328735
Chris Rorden's dcm2niiX version v1.0.20190902 (JP2:OpenJPEG) GCC8.3.0 (64-bit Linux)
Found 1 DICOM file(s)
Convert 1 DICOM as ./t_swi_acq-QSMX3echos_20200116145903_23_e3a (264x288x1x1)
Warning: Check that 2D images are not mirrored.
Conversion required 0.022960 seconds (0.022957 for core code).
$> grep Coil t_swi_acq-QSMX3echos_20200116145903_23_e3.json
"ReceiveCoilName": "Head_32",
"ReceiveCoilActiveElements": "HEA;HEP",
"CoilString": "H1",
$> grep Siem t_swi_acq-QSMX3echos_20200116145903_23_e3.json
"Manufacturer": "Siemens",
"PulseSequenceDetails": "%SiemensSeq%_gre",
There was a problem hiding this comment.
Honestly, I'm not familiar enough with the hardware to know if it's referring to the channel or the individual coil. All I can tell is that it's not referring to an array of coils like ReceiveCoilActiveElements.
- Add CoilString to `Scanner Hardware` table. - Change language for `ch` entity description.
@yarikoptic Would you prefer to use the actual label from |
I have no experience working with such data, so "not sure". To me it makes sense to use short label if such are available. Like we are not using |
Okay. It seems like something the BEP004 folks should weigh in on then. It would also be good to know what the labels for GE and Philips scanners tend to look like. |
|
I left an additional pointer to this PR in BEP004 google doc, hopefully they would chime in. Meanwhile IMHO we should decide either to have |
|
FWIW, I have inquired some friendly tech/physicists on Philips scanner -- I was told that there is (currently) no way to export DICOMs per each channel. It is possible to export raw uncombined data but in "raw formats" (.list/.data format, or .raw/.lab/.sin format)... and I said them to not bother. |
|
@effigies @bids-standard/bep_leads any feedback? |
|
Given that |
|
hm, isn't the file without |
|
@effigies @sappelhoff @robertoostenveld and @bids-standard/bep_leads -- what do you think about this PR? |
|
@tsalo -- btw travis isn't happy yet: |
|
As "co-lead" of BEP-001, I think this looks great. @yarikoptic, I agree that
Sorry, but not 100% sure why you bring up |
That's true, but I thought folks might want the ability to be explicit. If it would just cause confusion, then we don't need to have it. I won't allow "combined", but I will switch from index to label. |
tsalo
changed the title
I wouldn't explicitly disallow anything, +1 on switching to label (which who knows, might be combined if scanner gets a channel with such odd label ;-)) |
tsalo
changed the title
|
+1 |
|
|
Good catch @effigies. I completely forgot to define CoilString in the schema. I believe that Coil String is the actual name in the DICOM Tag, but I think it's vendor-specific and is not part of the DICOM specification. |
|
Maybe |
|
We should probably find out what different vendors use to label individual coils... Siemens data is the only one I have access to, and that's where Coil String comes from. If the others use the same name, I would go with that. Otherwise, I agree that a more descriptive name like CoilIdentifier would be better. |
|
@yarikoptic Is your scanner a Philips? |
|
@effigies I could maybe post in the BIDS Google Group asking for information about uncombined data across manufacturers? EDIT: Awesome! Posted in https://groups.google.com/g/bids-discussion/c/CK2sXdPXomE |
sorry, spotted just now. Used to be, now just Siemens. @neurolabusc might know more about all the Coil's metadata across manufacturers and/or have some sample uncombined acquisitions? |
|
BTW, there was some data shared in the course of nipy/heudiconv#424 (comment) : http://datasets.datalad.org/?dir=/dicoms/umass-philips/Zheng_Test_03302020 but not sure if it has it anyhow uncombined |
|
This one kind of languished, it seems due to a lack of multi-scanner exemplar data. Any thoughts on how to move forward @tsalo @yarikoptic? |
|
If there are any BIDS contributors who are able to generate or share some uncombined data, that would be really helpful. Unfortunately, I just don't have that ability at the moment. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #425 +/- ##
=======================================
Coverage 91.60% 91.60%
=======================================
Files 10 10
Lines 1060 1060
=======================================
Hits 971 971
Misses 89 89 ☔ View full report in Codecov by Sentry. |
The addition of this entity seems to address a rather specific niche case (which is ok), but the absence of forthcoming data over 2 years shows that this runs a high risk of being used rarely for the intended purpose, but more often overloaded with symbolic coil descriptors, as seen in #1170 . So perhaps |
|
😭 this PR has gone back and forth between Regarding the niche nature of the proposal, supporting uncombined data is, at minimum, necessary for the SWI BEP. That BEP was abandoned a couple of years ago, but I have to imagine that there are still neuroimagers interested in SWI and QSM, even if they aren't very vocal on the BIDS forums. |
Really sorry to come late to the entity party and relitigate this, but I'd argue this is a significant concern impacting potentially every single MR dataset; and we now have clear evidence that this overloading would happen. I'd be fine with anything other than
I didn't mean to cast doubt on the value of the enhancement, just that in conjunction with the name ambiguity this is something that's likely to be misapplied by anybody unfamiliar with the use case, and we might end up creating a different entity than what we think we're creating :3 |
Closes #370. This adds a new entity to distinguish coil-wise MRI data (i.e., data which have not been combined across coils).
An equivalent
coilentity is already proposed in BEP-004, but applies to more than SWI, as pretty much any data acquired on a multichannel coil could be reconstructed without being combined across coils.