bids-specification
bids-specification copied to clipboard
[ENH] BEP022 - Magnetic Resonance Spectroscopy
[!IMPORTANT]
This BEP is under community review from June 3 - June 14, 2024. Please participate at https://github.com/bids-standard/bids-specification/discussions/1846.
This pull request is to add BEP022 (magnetic resonance spectroscopy) to the BIDS specification.
Note that some of the MRI metadata descriptions have been modified to generalize to MRS or to make slightly different definitions apply to MRI and MRS.
[!Tip]
I'm not sure why the check_links test is failing. Seems to be a URL leading to a publication, but there isn't anything wrong with the link.
Had a quick look. I may be missing something but...
I am wondering if there is any reason to add the matrix size as a metadata when it is already implicitly present in the nifti header?
The description of VolumeAffineMatrix and AcquisitionVoxelSize make it clear that those values can actually be different from that found in the header. But this does not seem to be the case for the MatrixSize?
If so that it may lead to a duplication of information that the validator would need to check for consistency and software would need to know how to handle in case of conflict between the 2 values. So if MatrixSize is allowed to be different than the header value it may be good to use another name and update the description. If it is meant to be the same, then it may be better to remove it.
Sorry if I am missing something obvious.
It seems that this PR for the MRS modality introduces some metadata that could be reused by several if not all the datatypes under the MRI modality.
It may actually be a good idea to extract those from this PR and add them separately to the MRI common metadata in another PR.
This would:
- help make the MRS PR smaller by integrating a chunck of it early
- ensure (or at least try to maximize) metadata consistency between modalities
I can try to pick some of those metadata and prepare a draft PR.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 88.06%. Comparing base (
1235e5b
) to head (b110114
).
Additional details and impacted files
@@ Coverage Diff @@
## master #1377 +/- ##
=======================================
Coverage 88.06% 88.06%
=======================================
Files 16 16
Lines 1391 1391
=======================================
Hits 1225 1225
Misses 166 166
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Had a quick look. I may be missing something but...
I am wondering if there is any reason to add the matrix size as a metadata when it is already implicitly present in the nifti header?
The description of VolumeAffineMatrix and AcquisitionVoxelSize make it clear that those values can actually be different from that found in the header. But this does not seem to be the case for the MatrixSize?
If so that it may lead to a duplication of information that the validator would need to check for consistency and software would need to know how to handle in case of conflict between the 2 values. So if MatrixSize is allowed to be different than the header value it may be good to use another name and update the description. If it is meant to be the same, then it may be better to remove it.
Sorry if I am missing something obvious.
@Remi-Gau, you are correct that MatrixSize
is implicit to the nifti header. We can certainly remove it.
It seems that this PR for the MRS modality introduces some metadata that could be reused by several if not all the datatypes under the MRI modality.
It may actually be a good idea to extract those from this PR and add them separately to the MRI common metadata in another PR.
This would:
- help make the MRS PR smaller by integrating a chunck of it early
- ensure (or at least try to maximize) metadata consistency between modalities
I can try to pick some of those metadata and prepare a draft PR.
@Remi-Gau - agreed on this.
maintenance note: added a link to the rendered BEP in the top message of this PR.
@guiomar I've amended the PR based on most of your suggestions (some I still need to think about) - thanks so much for reviewing!
Assigning myself to remind me to go through with a fine-toothed comb next week. I think this is basically ready for community review, so I want to make sure we've got all our ducks in a row. We may need to set up a last meeting with maintainers/steering before review, but I need to remind myself of that process and any changes that may have been made to it since we started this.
Thanks very much, @markmikkelsen, for your endurance on this!
Hi @effigies (and cc'ing @wtclarke),
Responding to your two previous queries:
- Are there any metadata fields that are duplicated from the NIfTI header or the NIfTI-MRS header extension? Any duplication should be explicitly listed so that validators have a clear target. If the duplication is not exact but one is a transform of another or places a constraint, we should know those, too.
Metadata fields stored in the NIfTI-MRS header extension will depend on what can be extracted from the source dataset's header. (@wtclarke, could you confirm?). A number of them match the current MRS BEP metadata fields; others are not included in the BEP/BIDS. (Although, it would appear conversion software, like MRIcroGL, may also create non-BIDS metadata fields in their exported BIDSy-fied .json file.)
- Do any of the suffixes place constraints on the contents of the NIfTI files? For example I think
svs
should have a shape 1x1x1?
Yes, svs
data will have a shape of 1x1x1. A 2D mrsi
dataset's size could be something like 32x32x1. And a 3D mrsi
dataset's size could be 64x64x64.
Which suffixes can have a time dimension?
If you mean the equivalent to, say, the number of volumes in a BOLD-EPI scan, then, yes. This will be stored in one of dimensions 5-7.
Can any have a dimension >4?
Yes, dimensions >4 are possible, e.g., for dynamic repeats, uncombined RF channels, or subspectra (such as for edited data). But this will depend on the "rawness" of the source dataset that was converted.
For MRSI, must
MatrixSize == nifti_header.dim[1:4]
?
For MRSI, if you mean must MatrixSize == nifti_header.dim(2:4)
(MATLAB indexing syntax), then yes(?).
Yes, the exact content of the NIfTI-MRS header extension is dependant on input file type. Some file types that vendors output contain lots of information that can be interpreted, others the bare minimum. It also depends on what I can reliably interpret from the headers, as I am much less familiar with some vendor's formats. According to the NIfTI-MRS standard there is very little that is required, only SpectrometerFrequency and ResonantNucleus, and then any tags for higher dimensions (5th - 7th) that are in use.
Which suffixes can have a time dimension?
@Mark is getting at the fact that all NIfTI-MRS files have a time dimension (in the 4th NIfTI dimension). This (broadly) stores the time points of one FID/echo. A higher dimension (5-7) can then be labelled DIM_DYN
and be used to store repeated acquisitions.
@effigies Just updated mrs.yaml
and metadata.yaml
. Could you please check if it's what you had recommended?
Hi @markmikkelsen. Been organizing (and now attending thru Friday) a workshop for the last couple weeks, so apologies for being unable to respond in a timely manner. Will get back to this early next week. I would like to get this to the community review stage very soon. Would you like to have a call to sync up next week?
Hi @effigies, sounds good! I'll only be available next week Monday or Tuesday. Feel free to email me to set up a time.
Notes from call:
- We need to increase the requirement level for BodyPart+BodyPartDetails if the
voi
entity is present. You can useentities.volume
as a selector. - We need a check that ResonantNucleus and SpectrometerFrequency (both required fields) are defined in the MRS header extension. I will plan to submit a PR for this on Thursday afternoon.
- Please review the descriptions for EchoTime and FlipAngle and make sure that the wording makes sense in an MRS context.
- Set InversionTime to recommended if
inv
entity is present. (entities.inversion
) - Move to RepetitionTime and VolumeTiming, rather than RepetitionTime as an array in the variable-timing case.
- Add check that
sidecar.MatrixSize == nifti_header.dim[1:4]
, as in https://github.com/bids-standard/bids-specification/blob/181c50e08fd143d481e22be9a15c31f612e6f906/src/schema/rules/checks/func.yaml#L32-L43 - ReferenceSignal should be uniformly recommended.
- Examples should be updated to use
ReferenceSignal
andAnatomicalImage
where appropriate. - The fmrs example needs sidecar files.
Note for (6) that the expression language doesn't have slices, so that would be sidecar.MatrixSize[0] == nifti_header.dim[1] && ...
.
Hi @markmikkelsen. Just checking in that we're still on track for a review starting next Monday?
@effigies I've done another look over the BEP and, yes, I believe it's ready for review.
@markmikkelsen I've implemented all of the above suggestions (except 3, 8 and 9) in https://github.com/bids-standard/bids-specification/pull/1830. Please review that and merge if you're happy with the changes.
Hi @wtclarke and others. I hope this is the right place to comment.
-
Would you consider adding a "methodDoi" header to link to the DOI for the main paper describing the acquisition approach used? This may be a helpful historical record to bake into MRS data as there are so many one-off special methods. It could then aid an understanding of how the other parameters are (ab)used for new sequences beyond what is anticipated today.
-
Can the JSON note whether the MRS spatial coordinates are defined with or without distortion correction? I presume normally it's without distortion correction but not sure that is always the case. This will otherwise be a source of mis-alignment.
-
Does the specification offer guidance as to how to complete the Vendor, etc labels in cases of simulated data?
-
Does it provide a space to specify samples that should be dropped before analysis, e.g. initial points before a spin echo or initial points that are known to be corrupted during acquisition?
Thanks
Chris Rodgers.
@ctr Please create a new comment on https://github.com/bids-standard/bids-specification/discussions/1846.
- I think this will close https://github.com/bids-standard/bids-specification/issues/680, no?