bids-specification icon indicating copy to clipboard operation
bids-specification copied to clipboard

[ENH] BEP022 - Magnetic Resonance Spectroscopy

Open markmikkelsen opened this issue 2 years ago • 20 comments

[!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]

HTML preview of this BEP

markmikkelsen avatar Jan 06 '23 21:01 markmikkelsen

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.

markmikkelsen avatar Jan 06 '23 21:01 markmikkelsen

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 avatar Jan 27 '23 13:01 Remi-Gau

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 avatar Jan 27 '23 13:01 Remi-Gau

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.

codecov[bot] avatar Jan 27 '23 18:01 codecov[bot]

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.

markmikkelsen avatar Jan 27 '23 22:01 markmikkelsen

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.

markmikkelsen avatar Jan 27 '23 22:01 markmikkelsen

maintenance note: added a link to the rendered BEP in the top message of this PR.

Remi-Gau avatar Feb 13 '23 10:02 Remi-Gau

@guiomar I've amended the PR based on most of your suggestions (some I still need to think about) - thanks so much for reviewing!

markmikkelsen avatar Mar 01 '24 02:03 markmikkelsen

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!

effigies avatar Apr 11 '24 15:04 effigies

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(?).

markmikkelsen avatar Apr 23 '24 06:04 markmikkelsen

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.

wtclarke avatar Apr 23 '24 07:04 wtclarke

@effigies Just updated mrs.yaml and metadata.yaml. Could you please check if it's what you had recommended?

markmikkelsen avatar May 02 '24 04:05 markmikkelsen

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?

effigies avatar May 15 '24 14:05 effigies

Hi @effigies, sounds good! I'll only be available next week Monday or Tuesday. Feel free to email me to set up a time.

markmikkelsen avatar May 15 '24 18:05 markmikkelsen

Notes from call:

  1. We need to increase the requirement level for BodyPart+BodyPartDetails if the voi entity is present. You can use entities.volume as a selector.
  2. 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.
  3. Please review the descriptions for EchoTime and FlipAngle and make sure that the wording makes sense in an MRS context.
  4. Set InversionTime to recommended if inv entity is present. (entities.inversion)
  5. Move to RepetitionTime and VolumeTiming, rather than RepetitionTime as an array in the variable-timing case.
  6. 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
  7. ReferenceSignal should be uniformly recommended.
  8. Examples should be updated to use ReferenceSignal and AnatomicalImage where appropriate.
  9. 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] && ....

effigies avatar May 21 '24 20:05 effigies

Hi @markmikkelsen. Just checking in that we're still on track for a review starting next Monday?

effigies avatar May 28 '24 15:05 effigies

@effigies I've done another look over the BEP and, yes, I believe it's ready for review.

markmikkelsen avatar May 29 '24 06:05 markmikkelsen

@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.

effigies avatar May 29 '24 15:05 effigies

Hi @wtclarke and others. I hope this is the right place to comment.

  1. 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.

  2. 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.

  3. Does the specification offer guidance as to how to complete the Vendor, etc labels in cases of simulated data?

  4. 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 avatar Jun 03 '24 16:06 ctr

@ctr Please create a new comment on https://github.com/bids-standard/bids-specification/discussions/1846.

effigies avatar Jun 03 '24 17:06 effigies

  • I think this will close https://github.com/bids-standard/bids-specification/issues/680, no?

Remi-Gau avatar Aug 01 '24 17:08 Remi-Gau