-
Notifications
You must be signed in to change notification settings - Fork 182
[ENH] Update AcquisitionDuration definition to match DICOM, define FrameAcquisitionDuration for sparse sequences #1974
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
base: master
Are you sure you want to change the base?
Conversation
I've no issue proceeding with a definition that's not perfect for CT if there is not yet CT support in BIDS. I only mentioned it in #1906 to show that the inconsistency in definition of this parameter is not exclusively between DICOM and BIDS, but even within DICOM itself. |
I think this is fine. As the BIDS specifcation notes for SliceTiming |
d60fe83
to
c663490
Compare
b8ea0de
to
c6134f1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1974 +/- ##
=======================================
Coverage 82.15% 82.15%
=======================================
Files 17 17
Lines 1530 1530
=======================================
Hits 1257 1257
Misses 273 273 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ae28568
to
d3e46a3
Compare
Switch to using extension instead of nifti_header, to test metadata in the absence of headers. Make VolumeTiming-related errors contingent on the absence of RepetitionTime, since both being present is already an error.
d3e46a3
to
d461a12
Compare
This PR brings the AcquisitionDuration definition in line with DICOM and separates it from checks for sparse sequence descriptions. We already narrowed the AcquisitionDuration mutual exclusion rules to only apply to BOLD and ASL data.
This defines
FrameAcquisitionDuration
, which corresponds to DICOM tag 0018, 9220.@neurolabusc I wonder if this tag is used in any of your QA datasets yet?
@Lestropie Are you okay with deferring a CT-applicable definition to when CT data is accepted in BIDS? As noted in #1906 (comment), we do have mechanisms in the schema to provide multiple definitions for terms that are used in multiple contexts.
Closes #1906.
I would also like to relax the mutual exclusion constraints that are indicated in Task Imaging Data - Timing Parameters:
Given that
RepetitionTime
,SliceTiming
andFrameAcquisitionDuration
could all be present in the DICOM metadata, I think a better check would be for consistency among these, instead of enforcing mutual exclusion.DelayTime
andVolumeTiming
cannot be derived from DICOM tags (I don't think), so I would be okay with continuing to validate mutual exclusion or to validate consistency.RepetitionTime
andVolumeTiming
need to remain mutually exclusive, as they are distinct ways of declaring the onsets of each volume.I am happy to enact this change in this PR or another, if people agree that it's worth doing.