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

GE AcquisitionDuration #808

Closed
mr-jaemin opened this issue Mar 28, 2024 · 6 comments
Closed

GE AcquisitionDuration #808

mr-jaemin opened this issue Mar 28, 2024 · 6 comments

Comments

@mr-jaemin
Copy link
Collaborator

GE private DICOM tag (0019,105a) Acquisition Duration reports the duration of acquisition in micro-seconds, i.e. scan time for the series. I found that this information is useful.

I think that this tag is similar to DICOM 0018,9073 is BIDS AcquisitionDuration captupred #606

I tested this tag using Open MRI Consistency Data (specifically, Phantom02_DICOM.tar) and found that it matched with the console scan time for all series (localizer, t1 MPRAGE, fMRI, B0map, DTI)

For example, Sag_MPRAGE_T1

(0019,105a) FL 3.0118515e+08                            #   4, 1 AcquisitionDuration

--> 301.185 seconds
--> 05:01 (5 min 1 sec)

image

Console viewer (red arrow ) annotates the scan time in the 05:01 format

Screenshot 2024-03-27 at 11 34 16 PM

I would propose to populate these from GE (0019,105a) Acquisition Duration

"AcquisitionDuration": 301.185,
"AcquisitionDurationGE": "05:01",

Here are additional examples from Phantom02_DICOM.tar using a local commit:

02_Plane_Localizer.json:	"AcquisitionDurationGE": "00:17",
03_Sag_MPRAGE_T1.json:	"AcquisitionDurationGE": "05:01",
04_Standard_fBIRN_QA.json:	"AcquisitionDurationGE": "06:40",
05_rsfMRI_fieldmap_PE0.json:	"AcquisitionDurationGE": "00:15",
06_rsfMRI_fieldmap_PE1.json:	"AcquisitionDurationGE": "00:15",
07_3dB0map_rsfMRI_fieldmaphz.json:	"AcquisitionDurationGE": "01:00",
07_3dB0map_rsfMRI.json:	"AcquisitionDurationGE": "01:00",
08_rsfMRI_Run.json:	"AcquisitionDurationGE": "05:13",
09_DTI_PE1.json:	"AcquisitionDurationGE": "01:04",
10_3dB0map_fieldmaphz.json:	"AcquisitionDurationGE": "01:38",
10_3dB0map.json:	"AcquisitionDurationGE": "01:38",
11_DTI.json:	"AcquisitionDurationGE": "12:26",
12_3dB0map_fieldmaphz.json:	"AcquisitionDurationGE": "01:38",
12_3dB0map.json:	"AcquisitionDurationGE": "01:38",

02_Plane_Localizer.json:	"AcquisitionDuration": 17.2842,
03_Sag_MPRAGE_T1.json:	"AcquisitionDuration": 301.185,
04_Standard_fBIRN_QA.json:	"AcquisitionDuration": 400,
05_rsfMRI_fieldmap_PE0.json:	"AcquisitionDuration": 14.8,
06_rsfMRI_fieldmap_PE1.json:	"AcquisitionDuration": 14.8,
07_3dB0map_rsfMRI_fieldmaphz.json:	"AcquisitionDuration": 60.3984,
07_3dB0map_rsfMRI.json:	"AcquisitionDuration": 60.3984,
08_rsfMRI_Run.json:	"AcquisitionDuration": 312.8,
09_DTI_PE1.json:	"AcquisitionDuration": 63.9,
10_3dB0map_fieldmaphz.json:	"AcquisitionDuration": 97.9132,
10_3dB0map.json:	"AcquisitionDuration": 97.9132,
11_DTI.json:	"AcquisitionDuration": 745.5
12_3dB0map_fieldmaphz.json:	"AcquisitionDuration": 97.9132,
12_3dB0map.json:	"AcquisitionDuration": 97.9132,
mr-jaemin pushed a commit to mr-jaemin/dcm2niix that referenced this issue Mar 28, 2024
@mr-jaemin
Copy link
Collaborator Author

Please @neurolabusc review the proposal or the above commit. I will submit the PR once you gives okay.

@neurolabusc
Copy link
Collaborator

@mr-jaemin I have two minor comments.

First, your premise is that the private tag 0019,105A is more accurate than the public tag 0018,9073. Therefore, I would ensure that the private tag always gets precedence. While tags are typically provided sequentially, this may not be the case for nested details. Therefore, I would change nii_dicom.cpp:

		case kAcquisitionDuration:
			if (!isSameFloatGE(d.acquisitionDuration, 0.0))
				break; //issue 808: give precedence to more precise measures
			//n.b. used differently by different vendors https://github.com/rordenlab/dcm2niix/issues/225
			d.acquisitionDuration = dcmFloatDouble(lLength, &buffer[lPos], d.isLittleEndian);
			break;

Second, I would not add the AcquisitionDurationGE field to nii_dicom_batch.cpp as it is completely redundant with AcquisitionDuration (though rounded so it stores precision).

If you implement my second comment, you will delete the sprintf() call from your pull request. I recognize that your usage of sprintf is safe, but please use snprintf() instead to remove distracting clang/LLVM compiler warnings. Specifically, clang will report warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. While @yhuang43 and the FreeSurfer team have added a few sprintf() calls, they all require the USING_DCM2NIIXFSWRAPPER compilation flag and therefore do not impact the regular compilation.

@mr-jaemin
Copy link
Collaborator Author

@neurolabusc Thanks for your comments.

  • Good point to give precedence to more precise measures. Added some details
case kAcquisitionDuration:
   if (!isSameFloatGE(d.acquisitionDuration, 0.0))
      break; //issue 808: give precedence to more precise measures, e.g kAcquisitionDurationGE (0019,105a)
  • Agreed that MM:SS format (AcquisitionDurationGE) is completely redundant with AcquisitionDuration in seconds.
    • I personally find this human readable format (MM:SS) quite useful to check the scan time. Will think/discuss more.
    • Regardless, thanks for the snprintf() suggestion (didn't know this compiler issue)
    • Any suggestion? C++ string like std:string vs C-style string like this example, in general?

@brice82
Copy link

brice82 commented Mar 28, 2024

Hi @neurolabusc and @mr-jaemin,

If I may make a little suggestions, even though this is nice have this field in a human readable format, it is maybe not desired in every situation (and this is very GEHC specific).

As there is already some specific fields enable only when a certain compilation flag is enabled, maybe we can create a GEHC specific compile flag.

for example in

#ifdef MY_DEBUG

There is MY_DEBUG flag for some GEHC specific parameters.
Maybe, we could make a compile flag like GEHC_EXTRA_FIELD_FLAG to have in addition AcquisitionDurationGE, and also the already existing fields lke EchoSpacingMicroSecondsGE , NotPhysicalNumberOfAcquiredPELinesGE and NotPhysicalTotalReadOutTimeGE.

What do you think ? what that be reasonable ?

@neurolabusc
Copy link
Collaborator

My sense is to add these features to all version of dcm2niix, so different users do not get different details. I do think that we should not included redundant tags where they include the same informatin (AcquisitionDuration vs AcquisitionDurationGE), we should use private tags when they provide more precise information than public tags (0019,105A vs 0018,9073), and we should include sequence details in the JSON files if they may aid subsequent processing (EchoSpacingMicroSecondsGE , NotPhysicalNumberOfAcquiredPELinesGE and NotPhysicalTotalReadOutTimeGE). Happy to meet and build consensus if there is disagreement on these thoughts.

mr-jaemin pushed a commit to mr-jaemin/dcm2niix that referenced this issue Mar 29, 2024
@mr-jaemin
Copy link
Collaborator Author

mr-jaemin commented Mar 29, 2024

Agreed. Since both DICOM and BIDS AcquisitionDuration are in seconds, I would remove a redundant AcquisitionDurationGE (MM:SS) tag.

submitted PR #810

In summary,

  • While GEHC doesn't report public tag (0018,9073) AcquisitionDuration yet,
  • GEHC private tag (0019,105A) reports AcquisitionDuration in micro-seconds:
    (0019,105a) FL 3.0118515e+08 # 4, 1 AcquisitionDuration
  • dcm2niix (v1.0.20240327+) extracts the private tag (0019,105A) which gets precedence over the (0018,9073)
  • Report in the JSON files
    "AcquisitionDuration": 301.185,

neurolabusc added a commit that referenced this issue Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants