-
Notifications
You must be signed in to change notification settings - Fork 170
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 ParallelReductionFactorOutOfPlane to MRI metadata #1221
Conversation
Codecov ReportBase: 88.39% // Head: 88.39% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #1221 +/- ##
=======================================
Coverage 88.39% 88.39%
=======================================
Files 11 11
Lines 1086 1086
=======================================
Hits 960 960
Misses 126 126 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
This makes sense to me. I think it might make sense to change the heading for the table to just "Spatial encoding" and include the And what do you think about making the requirement level |
@effigies I would be fine with combining the tables into one table under the heading "Spatial encoding". I was just trying to be minimally invasive as to what was there before. Regarding making the requirement level conditional, how about Let me know what you think, and I will have a go at implementing these changes. |
My concern here is that if we take RECOMMENDED and OPTIONAL seriously, they have the following definitions:
Consequently, the validator is beginning to warn on more missing RECOMMENDED values and we should be careful about adding to the list of fields that will alert the user. Looking at the DICOM standard, the out-of-plane metadata is REQUIRED/OPTIONAL in the same conditions that In-plane is, so I think it's reasonable for them to have the same requirement level, which would be RECOMMENDED. I suppose my remaining question is if you know if this tag is ubiquitous in practice? cc @neurolabusc for thoughts |
src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some suggestions, as the tables are now generated directly from the rules/sidecars/*.yaml
. See the rendered doc here: https://bids-specification--1221.org.readthedocs.build/en/1221/04-modality-specific-files/01-magnetic-resonance-imaging-data.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @effigies for pushing this forward. As the new metadata field is "RECOMMENDED", I added a brief clarification that ParallelReductionFactorOutOfPlane
should typically be 1 for 2D sequences, and a brief bit of disambiguating text to distinguish it from MultibandAccelerationFactor
.
Otherwise it is all good from my side.
According to @neurolabusc's |
@bids-standard/maintainers Anybody up for a second review? |
This pull request adds the attribute
"ParallelReductionFactorOutOfPlane"
(corresponding to the DICOM tag (0018,9155)"Parallel Reduction Factor out-of-plane"
) to the MRI schema to allow proper specification of parallel imaging in the out of plane direction.This is necessary because 3D MRI sequences are often acquired with parallel imaging applied in both the in plane and out of plane directions, for example in this open dataset: https://doi.org/10.1016/j.dib.2019.104132, where an acceleration factor of 2 was used in plane and also out of plane (2 x 2 GRAPPA). This cannot be captured with the current schema, where only
"ParallelReductionFactorInPlane"
is defined.Please note that
"ParallelReductionFactorOutOfPlane"
is distinct from and independent of the"MultibandAccelerationFactor"
, which defines acceleration by acquiring multiple non-adjacent slices in 2D sequences.