bids-specification icon indicating copy to clipboard operation
bids-specification copied to clipboard

[ENH] Update AcquisitionDuration definition to match DICOM, define FrameAcquisitionDuration for sparse sequences

Open effigies opened this issue 1 year ago • 3 comments

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 https://github.com/bids-standard/bids-specification/issues/1906#issuecomment-2436578444, 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:

image

Given that RepetitionTime, SliceTiming and FrameAcquisitionDuration 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 and VolumeTiming 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 and VolumeTiming 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.

effigies avatar Nov 01 '24 15:11 effigies

Are you okay with deferring a CT-applicable definition to when CT data is accepted in BIDS?

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.

Lestropie avatar Nov 03 '24 22:11 Lestropie

I think this is fine. As the BIDS specifcation notes for SliceTiming Without this parameter slice time correction will not be possible, so that provides more granularity than the other options. I do wonder if we should include a graphic or at lest a link to the DICOM Figure C.7.6.16-2. Hopefully the fence post problem is clear with the existing definitions, e.g. the final slice time is the onset time for the final slice in a 3D volume relative to the onset time of the first slice in the volume. In contrast, Acquisition Duration spans the start of the first temporal slice to the completion of the final slice.

neurolabusc avatar Nov 05 '24 14:11 neurolabusc

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 82.15%. Comparing base (8e175f4) to head (d461a12).

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           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 04 '25 18:06 codecov[bot]

This has two approvals and the critiques have gone silent. Will merge, and we can back it out if that motivates critics to speak up again.

effigies avatar Aug 04 '25 19:08 effigies