Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ENH] BEP022 - Magnetic Resonance Spectroscopy #1377

Merged
merged 111 commits into from
Aug 26, 2024
Merged

[ENH] BEP022 - Magnetic Resonance Spectroscopy #1377

merged 111 commits into from
Aug 26, 2024

Conversation

markmikkelsen
Copy link
Contributor

@markmikkelsen markmikkelsen commented Jan 6, 2023 •
edited by effigies
Loading

Important

This BEP is under community review from June 3 - June 14, 2024. Please participate at #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

Closes #680.

@markmikkelsen
First commit of bep022 into BIDS
fc31bac 
Adding and changing files to introduce MRS to the BIDS specification.
@markmikkelsen
More additions for bep022
3f7b06f 
- Added future publication ref in 01-introduction.md
- Added table of MRS sequences
- Starting to add new details for the MRS JSON sidecar
- Added MRS-MRI correspondence in Appendix
- Added volume as a new entity
- Changed order of MRS suffixes in src/schema/rulesfiles/raw/mrs.yaml
@markmikkelsen
Further updates for bep022
04541ec
@markmikkelsen
Further updates
7a8a3af
@markmikkelsen
Adding more metadata to MRS-specific fields
2a644aa
@markmikkelsen
Finalizing additions for bep022
eed04ce
@markmikkelsen
Minor update
505a75f
@markmikkelsen
Spelling correction
dc65a36
@markmikkelsen
Update contributors.md
1979532
@markmikkelsen
Some minor edits
81b057b
@markmikkelsen
Copy link
Contributor Author

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.

@effigies effigies changed the title [ENH] Add BEP022 (magnetic resonance spectroscopy) to the specification BEP022 - Magnetic Resonance Spectroscopy Jan 6, 2023
@sappelhoff sappelhoff added the BEP label Jan 11, 2023
@Remi-Gau
Copy link
Collaborator

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.

@markmikkelsen
Copy link
Contributor Author

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

@effigies
Copy link
Collaborator

@markmikkelsen I've implemented all of the above suggestions (except 3, 8 and 9) in #1830. Please review that and merge if you're happy with the changes.

markmikkelsen and others added 14 commits May 29, 2024 14:02
@markmikkelsen
Update metadata.yaml
2c969bb 
- Spelling correction
@markmikkelsen @effigies
Update src/schema/objects/metadata.yaml
8fc41ea 
Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>
@markmikkelsen @effigies
Update src/schema/objects/metadata.yaml
9116def 
Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>
@markmikkelsen
Merge branch 'bids-standard:bep022' into bep022
40bae63
@markmikkelsen
Merge branch 'master' into bep022
0b03476
@effigies
SCHEMA: Add nifti_header.mrs to context
f6e3f72
@effigies
SCHEMA: Add check for consistency between MRS header and sidecar
899e6ec
@effigies
Split conditional matrices in schema, combine in doc
9dabf8e 
This implements the following agreed upon suggestions:

* 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.
* ReferenceSignal should be uniformly recommended.
@effigies
Make BodyPart(Details) required if voi entity is present
07d2ec8
@effigies
feat(checks): Add check to ensure MatrixSize and nifti_header.dim are…
0181150 
… consistent
@effigies
fix(checks): Use non-trivial expressions for selectors
83c6f3e
@effigies
Uncombine required/recommended/optional fields
1d5e089
@markmikkelsen
Merge pull request #1830 from effigies/schema/mrs_header
e1ae75e 
[SCHEMA] MRS schema updates
@markmikkelsen
Merge branch 'master' into bep022
70bc2ca
@ctr
Copy link

ctr commented Jun 3, 2024

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.

@effigies
Copy link
Collaborator

effigies commented Jun 3, 2024

@ctr Please create a new comment on #1846.

markmikkelsen and others added 2 commits July 25, 2024 16:53
@markmikkelsen @tsalo
Update src/schema/rules/sidecars/func.yaml
b0bea61 
Co-authored-by: Taylor Salo <salot@pennmedicine.upenn.edu>
@effigies
FIX: Add MRS events files, restrict sidecar metadata checks to data f…
1ef102b 
…iles (#1875)

* FIX: Add MRS events files, restrict sidecar metadata checks to data files

* FIX: Do not limit suffixes for filename template
@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Aug 1, 2024

@effigies effigies merged commit 93b6d1f into master Aug 26, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bpinsard bpinsard bpinsard left review comments

@guiomar guiomar guiomar left review comments

@Remi-Gau Remi-Gau Remi-Gau left review comments

@tsalo tsalo tsalo left review comments

@wtclarke wtclarke wtclarke left review comments

@effigies effigies effigies approved these changes

@erdalkaraca erdalkaraca Awaiting requested review from erdalkaraca erdalkaraca is a code owner

Assignees

@effigies effigies

Labels
BEP spectroscopy
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

Extend BIDS format to magnetic resonance spectroscopy
9 participants
@markmikkelsen @Remi-Gau @effigies @wtclarke @ctr @bpinsard @guiomar @tsalo @sappelhoff