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.

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

[ENH] Add "chunk" entity to MRI datatype #1586

Merged

Conversation

valosekj
Copy link
Contributor

The chunk entity was originally added for the 'Microscopy' datatype in BEP031 #881. This PR adds the chunk entity also to the MRI datatype. In the context of MRI, chunks refer to individual field-of-views (FOVs), for example, one FOV for the brain and the second FOV for the spinal cord.
Since two different FOVs might be acquired not only for anatomical MRI, I added the chunk entity also for DWI and fMRI.

Resolves: #1382

@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (466e212) 87.83% compared to head (f6282cc) 87.83%.

❗ Current head f6282cc differs from pull request most recent head 6dc1e30. Consider uploading reports for the commit 6dc1e30 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1586   +/-   ##
=======================================
  Coverage   87.83%   87.83%           
=======================================
  Files          16       16           
  Lines        1356     1356           
=======================================
  Hits         1191     1191           
  Misses        165      165           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Remi-Gau
Copy link
Collaborator

@valosekj would it be possible to work on an example dataset to add to bids-example like it was done when the task entity was added for anat: bids-standard/bids-examples#341?

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadly makes sense. Do we want to bring over any of the sample metadata (such as BodyPart) from micr? If the main purpose in MRI is to distinguish spinal cord from brain, annotating that consistently would presumably be helpful.

MicroscopySample:
selectors:
- modality == "micr"
- datatype == "micr"
- suffix != "photo"
fields:
BodyPart:
level: recommended
description_addendum: |
From [DICOM Body Part
Examined](http://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_L.html#chapter_L)
(for example `"BRAIN"`).
BodyPartDetails: recommended
BodyPartDetailsOntology: optional
SampleEnvironment: recommended
SampleEmbedding: optional
SampleFixation: optional
SampleStaining: recommended
SamplePrimaryAntibody: recommended
SampleSecondaryAntibody: recommended
SliceThickness: optional
TissueDeformationScaling: optional
SampleExtractionProtocol: optional
SampleExtractionInstitution: optional

It would go into https://github.com/bids-standard/bids-specification/blob/master/src/schema/rules/sidecars/mri.yaml.

Also, would this apply to fieldmap files as well?

src/schema/objects/metadata.yaml Outdated Show resolved Hide resolved
src/schema/objects/entities.yaml Outdated Show resolved Hide resolved
@Remi-Gau
Copy link
Collaborator

Note. Bringing some of the metadata like bodypart may be easier to do on its own and done via a separate PR.

@effigies
Copy link
Collaborator

See #1593.

Co-authored-by: Chris Markiewicz <[email protected]>
@valosekj

This comment was marked as outdated.

@effigies
Copy link
Collaborator

Examples should be submitted as PRs to https://github.com/bids-standard/bids-examples/.

Please see https://github.com/bids-standard/bids-examples/blob/master/CONTRIBUTING.md for guidance.

@valosekj
Copy link
Contributor Author

@valosekj would it be possible to work on an example dataset to add to bids-example like it was done when the task entity was added for anat: bids-standard/bids-examples#341?

Example dataset added in bids-standard/bids-examples#405

@valosekj
Copy link
Contributor Author

@effigies and @Remi-Gau, is anything else required, or can this PR be merged?
Note that I will bring some of the metadata like bodypart in a separate PR, as proposed in #1586 (comment).

@Remi-Gau
Copy link
Collaborator

I think that @effigies comment is worth considering:

would this apply to fieldmap files as well?

Don't know how much sense it would make for ASL but for fieldmap it may make sense to support chunck: even if in theory we could just rely on the IntendedFor field to say that a given fieldmap is for this or that chunk it may make it more explicit if we allow this in the filename.

@jcohenadad
Copy link
Contributor

jcohenadad commented Sep 12, 2023

Don't know how much sense it would make for ASL but for fieldmap it may make sense to support chunck: even if in theory we could just rely on the IntendedFor field to say that a given fieldmap is for this or that chunk it may make it more explicit if we allow this in the filename.

Yes, good point. It is not uncommon to acquire different chunks of EPIs, with their associated B0 fieldmap.

@valosekj
Copy link
Contributor Author

I think that @effigies comment is worth considering:

would this apply to fieldmap files as well?

Don't know how much sense it would make for ASL but for fieldmap it may make sense to support chunck: even if in theory we could just rely on the IntendedFor field to say that a given fieldmap is for this or that chunk it may make it more explicit if we allow this in the filename.

Good point! I added the chunk entity to fieldmaps in 901fe7f.
Also, I added chunk to multiecho in f6282cc (I originally missed it).

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be extended to ASL images as well? We should not hold up on that.

@Remi-Gau
Copy link
Collaborator

Should this be extended to ASL images as well?

I am struggling to imagine how ASL on several chunks would be done in a way that "make sense", but one could also say the same for func. Also I have been known to lack imagination...
So I am fine either way.

We should not hold up on that.

Whether it should be added or not should not stop from merging this PR now.

@effigies
Copy link
Collaborator

For BOLD and ASL, I think you would stitch together statistical results, not individual volumes.

@effigies effigies merged commit f6bcb0a into bids-standard:master Sep 25, 2023
16 checks passed
@Remi-Gau
Copy link
Collaborator

For BOLD and ASL, I think you would stitch together statistical results, not individual volumes.

when I said I lacked imaganitation I did not mean you should prove it to me right away. 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add chunk entity to MRI datatype
4 participants