-
Notifications
You must be signed in to change notification settings - Fork 190
[FIX] Rescope or downgrade excess RECOMMENDED fields #2116
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
[FIX] Rescope or downgrade excess RECOMMENDED fields #2116
Conversation
86a62a7 to
5d0af72
Compare
9e5dd42 to
572364b
Compare
572364b to
09d4114
Compare
nellh
left a comment
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.
I did some testing of this with example datasets and everything here looks good to me.
effigies
left a comment
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.
Some notes for reviewers with links to impacts on the rendered spec.
| {{ MACROS___make_sidecar_table([ | ||
| "mri.MRIB0FieldIdentifier", | ||
| "fmap.MRIFieldmapB0FieldIdentifier", | ||
| "mri.MRIEchoPlanarImagingAndB0FieldSource", | ||
| ]) | ||
| }} |
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.
Before
| PhaseEncodingDirectionReq: | ||
| selectors: | ||
| - datatype == "fmap" | ||
| - suffix == "epi" | ||
| - match(extension, "^\.nii(\.gz)?$") | ||
| fields: | ||
| PhaseEncodingDirection: | ||
| level: required | ||
| issue: | ||
| code: PHASE_ENCODING_DIRECTION_MUST_DEFINE | ||
| message: | | ||
| You have to define 'PhaseEncodingDirection' for this file. | ||
| TotalReadoutTime: | ||
| level: required | ||
| description_addendum: <sup>3</sup> | ||
| issue: | ||
| code: TOTAL_READOUT_TIME_MUST_DEFINE | ||
| message: | | ||
| You have to define 'TotalReadoutTime' for this file. | ||
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.
Duplicates table in fmap.yaml.
| MRIB0FieldIdentifier: | ||
| selectors: | ||
| - datatype == 'fmap' | ||
| - match(extension, '\.nii(\.gz)?$') | ||
| fields: | ||
| B0FieldIdentifier: recommended | ||
|
|
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.
Duplicates table in fmap.yaml.
| MRIPartialFourier: | ||
| selectors: | ||
| - modality == "mri" | ||
| - match(extension, "^\.nii(\.gz)?$") | ||
| - type(sidecar.PartialFourier) != "null" | ||
| fields: | ||
| PartialFourierDirection: recommended | ||
|
|
||
| MRIParallelReductionFactorInPlane: | ||
| selectors: | ||
| - modality == "mri" | ||
| - match(extension, "^\.nii(\.gz)?$") | ||
| - type(sidecar.ParallelAcquisitionTechnique) == "string" | ||
| fields: | ||
| ParallelReductionFactorInPlane: recommended | ||
|
|
||
| MRIParallelReductionFactorOutOfPlane: | ||
| selectors: | ||
| - modality == "mri" | ||
| - match(extension, "^\.nii(\.gz)?$") | ||
| - type(sidecar.ParallelAcquisitionTechnique) == "string" | ||
| - sidecar.MRAcquisitionType == "3D" | ||
| fields: | ||
| ParallelReductionFactorOutOfPlane: recommended |
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.
Conditional promotions of fields from OPTIONAL to RECOMMENDED, based on other sidecar fields.
| EpochLength: | ||
| level: optional | ||
| # Implemented in rules.sidecars.electrophys | ||
| level_addendum: recommended if RecordingType is "epoched" |
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.
| (which can be later used for field inhomogeneity correction). | ||
| TotalReadoutTime: | ||
| level: recommended | ||
| description_addendum: <sup>3</sup> |
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.
This was misplaced in the table being removed below and not rendered before. Now:
| InversionTime: recommended | ||
| InversionTime: | ||
| level: optional | ||
| level_addendum: required if `inv` entity is present |
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.
| SliceEncodingDirection: recommended | ||
| SliceEncodingDirection: optional |
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.
| fields: | ||
| MultibandAccelerationFactor: recommended | ||
| MultibandAccelerationFactor: optional |
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.
| B0FieldSource: | ||
| level: recommended | ||
| issue: | ||
| code: B0_FIELD_SOURCE_RECOMMENDED | ||
| message: | | ||
| File is missing B0FieldSource metadata, so it may not be possible to | ||
| perform susceptibility distortion correction on it. | ||
| B0FieldIdentifier metadata permits the flexible selection of images | ||
| for estimating B0 inhomogeneity fields, and B0FieldSource permits EPI | ||
| images to indicate the identifier of the estimated field to use. | ||
| If associations are fully specified with `IntendedFor`, there is no need to change this. |
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.
Converts generic sidecar warning to a more helpful message that indicates when it can be safely ignored.
|
This looks good to me. I know that we had recurring conversations when working on the MRS BEP about which fields should be |



Adopts the recommendations from #2040 (comment).
Closes #2040.
Closes bids-standard/bids-validator#183.