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

[BUG] Inconsistent definition of AcquisitionDuration #1906

Open
effigies opened this issue Aug 22, 2024 · 5 comments · May be fixed by #1974
Open

[BUG] Inconsistent definition of AcquisitionDuration #1906

effigies opened this issue Aug 22, 2024 · 5 comments · May be fixed by #1974
Assignees
Labels
bug Something isn't working
Milestone

Comments

@effigies
Copy link
Collaborator

effigies commented Aug 22, 2024

The current definition of AcquisitionDuration is

Duration (in seconds) of volume acquisition. Corresponds to DICOM Tag 0018, 9073 Acquisition Duration. This field is mutually exclusive with "RepetitionTime".

The definition of DICOM tag 0018, 9073 is:

The time in seconds needed to run the prescribed pulse sequence.

They also helpfully provide a graphic to clarify the definition:

So this is the duration of the full scan, which only overlaps for single-volume acquisitions. Our definition more closely aligns with
FrameAcqusitionDuration 0018,9220:

The actual amount of time [in milliseconds] that was used to acquire data for this frame.


I propose the following (re)definitions:

AcquisitionDuration: Duration, in seconds, of scan acquisition, including all volumes for multi-volume scans. Corresponds to DICOM Tag 0018, 9073 Acquisition Duration.
FrameAcquisitionDuration: Duration, in seconds, of volume acquisition. Corresponds to DICOM Tag 0018,9220 Frame Acquisition Duration.

This change needs to be communicated somehow. While upstream tools are going to refer to DICOM, downstream tools may only refer to BIDS. Any suggestions for wording?

@Lestropie
Copy link
Collaborator

Even the DICOM definitions might not be ideal:

  • "Time in seconds needed to run the prescribed pulse sequence" is very MR-centric
  • "The time in seconds needed to complete the acquisition of data" (found from same DICOM tag but for CT) might be misconstrued as commencing on the first TR where data corresponding to the output DICOM series are "acquired", ie. excluding establishing steady-state / calibration volumes

I wonder if there's something in the realm of "the time required to execute the program responsible for image data acquisition"; that would exclude sequence-agnostic features like patient prep / re-shimming, but rightly include things like establishing magnetisation steady-state.

@Remi-Gau Remi-Gau added the bug Something isn't working label Oct 18, 2024
@effigies effigies added this to the 1.10.1 milestone Oct 24, 2024
@effigies
Copy link
Collaborator Author

Short term fix is to add selectors to only apply the mutex for BOLD images:

RepetitionTimeAcquisitionDurationMutex:
issue:
code: REPETITION_TIME_AND_ACQUISITION_DURATION_MUTUALLY_EXCLUSIVE
message: |
The fields 'RepetitionTime' and 'AcquisitionDuration' for this file are mutually exclusive.
To specify acquisition duration, use 'SliceTiming' or 'DelayTime'
(RepetitionTime - AcquisitionDuration).
level: error
selectors:
- type(sidecar.AcquisitionDuration) != "null"
checks:
- type(sidecar.RepetitionTime) == "null"

@effigies
Copy link
Collaborator Author

effigies commented Oct 25, 2024

Even the DICOM definitions might not be ideal:

  • "Time in seconds needed to run the prescribed pulse sequence" is very MR-centric

  • "The time in seconds needed to complete the acquisition of data" (found from same DICOM tag but for CT) might be misconstrued as commencing on the first TR where data corresponding to the output DICOM series are "acquired", ie. excluding establishing steady-state / calibration volumes

I wonder if there's something in the realm of "the time required to execute the program responsible for image data acquisition"; that would exclude sequence-agnostic features like patient prep / re-shimming, but rightly include things like establishing magnetisation steady-state.

After a couple months, I'm still undermotivated to come up with a sentence that satisfies multiple modalities. We can use AcquisitionDuration__mri and AcquisitionDuration__ct to distinguish between two different interpretations of the same concept. As long as we point back to the DICOM tag and show the right text for MRI and CT, I think the situation is unambiguous.

@yarikoptic
Copy link
Collaborator

  1. @Lestropie

... but rightly include things like establishing magnetisation steady-state.

why would we want to capture that? IMHO such data does not fall under "Acquisition" since

  • no data (visible to use) was acquired during that procedure
  • even in DICOM diagram those durations are not part of the AcquisitionDuration
  • it is of no immediate use/interest for BIDS users

1.b If we were looking to capture all those, could be some SequenceDuration or alike.

  1. I would propose adjustment to the suggested phrase to become

"The time in seconds spent on acquisition of data"

since "needed to complete" might imply all the preparation, and who knows - may be some "cool down" if there was any, etc.

  1. re @effigies

This change needs to be communicated somehow.

I feel that this relates to ability to migrate or upgrade BIDS datasets across versions. Proposed here is a "breaking change", which if released in e.g. 2.0.0 release, we could provide upgrade path. Related:

In principle it could be implemented earlier, but I think then no amount of documentation would suffice since it might be too intrusive and "violate" the promise of compatibility within 1.x series.

So what about slating it for 2.0.0 release?

  1. This loosely relates to our findings about odd timing of the AcquisitionTime we are exploring with @bpinsard @vmdocua et al in Oddity of "jumping offset" of DICOM's AcquisitionTime across sequences ReproNim/reprostim#109 -- where apparently that time point does not correspond to the time when trigger pulse sent to signal actual data acquisition start.

@Lestropie
Copy link
Collaborator

why would we want to capture that?

My thinking was less so about what one might "want to capture" but rather what the contents of the pre-existing DICOM header field encodes, is therefore likely to appear in JSON data that have undergone conversion from DICOM somewhat agnostic to BIDS, and therefore what the BIDS description of that field should be to be faithful. Having a BIDS field where the name is identical to a DICOM field, but the intended contents are different, is probably asking for trouble (and indeed the raison d'etre of this Issue).

even in DICOM diagram those durations are not part of the AcquisitionDuration

I think this might be a misunderstanding of the diagram:

Image

The time taken to reach steady-state magnetisation is the green highlight. That is specific to the particular pulse sequence being executed, and is included as part of the DICOM field AcquisitionDuration. What this looks like and how long it takes will depend on the sequence / parameters provided to that sequence. The parts that are not included in DICOM AcquisitionDuration is the red highlight, which is what I referred to previously as "patient prep / re-shimming"; this is everything that happens prior to the scanner saying eg. "execute the program called MPRAGE".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants