rordenlab / dcm2niix

dcm2nii DICOM to NIfTI converter: compiled versions available from NITRC
https://www.nitrc.org/plugins/mwiki/index.php/dcm2nii:MainPage
Other
874 stars 229 forks source link

BIDS sidecar field ParallelReductionFactorInPlane too large #672

Closed lukeje closed 1 year ago

lukeje commented 1 year ago

Describe the bug

The reported ParallelReductionFactorInPlane in the JSON sidecar file for Siemens data is the product of the in-plane and out-of-plane parallel reduction factors.

I think the problem occurs because accelFactPE (which is the source of ParallelReductionFactorInPlane for Siemens data) is read from a string which contains the total acceleration, not just the in-plane acceleration.

To reproduce

Steps to reproduce the behavior:

  1. Download the B1+_mapping DICOM dataset from here, measured on a Siemens scanner with GRAPPA 2×2 (i.e. it should give ParallelReductionFactorInPlane = 2, ParallelReductionFactorOutOfPlane = 2)
  2. Run dcm2niix on the DICOM files
  3. See that the ParallelReductionFactorInPlane is 4 in the BIDS sidecar files, rather than 2.

Expected behavior

Ideally both ParallelReductionFactorInPlane and ParallelReductionFactorOutOfPlane would be 2, rather than ParallelReductionFactorInPlane being the product of the two in the JSON sidecar files. Note that AccelFact3D in the BIDS sidecar is 2, as expected.

Note that this data does come from a "user" sequence, but based on this comment in the dcm2niix source code, this is not the only dataset where this is a problem.

Version

Tested with the current master branch:

Tested with the latest development branch:

Troubleshooting

Please try the following steps to resolve your issue:

It is the latest stable release.

No, the development branch did not resolve the issue.

neurolabusc commented 1 year ago

@lukeje can you communicate with your Siemens Research Collaboration manager to help out with this. The DICOM header is not internally consistent.

While the CSA header suggests ParallelReductionFactorInPlane = 2 and ParallelReductionFactorOutOfPlane = 2:

sPat.lAccelFactPE    =  2
sPat.lAccelFact3D    =  2

The private tag suggests ParallelReductionFactorInPlane = 4 and ParallelReductionFactorOutOfPlane = 1:

(0051,1011) LO [p4]

whereas the CSA would lead one to expect to see:

(0051,1011) LO [p2s2]

Perhaps this was an early E11 release when SMS was still new and not given its own acceleration flag.

In general, dcm2niix assumes DICOM headers are consistent and truthful. We can always reverse the precedence between 0051,1011 and the CSA header. However, I would want to understand the reason for the conflict to make sure there are no unintended consequences.

lukeje commented 1 year ago

I just checked a VE12U dataset recorded with a ported version of the sequence and the problem seems to remain there too, i.e. with GRAPPA 2×2 the string in the DICOM is:

DICOM/SIEMENS CSA HEADER/ImaPATModeText: p4(string)
DICOM/SIEMENS CSA HEADER/PATModeText: p4(string)

This doesn't rule out that Siemens product sequences might have fixed the inconsistency in VE12, though; I'll check when I'm next at the scanner.

lukeje commented 1 year ago

Looking again at the dcm2niix source code, it looks like even if the out-of-plane GRAPPA acceleration factor was stored as M in the CSA string pNsM, it would be assumed to be the (2D) multiband factor, rather than the (3D) out-of-plane acceleration factor. Once I've got an example dataset from a product sequence I can try and reach out to someone at Siemens to see what interpretation of this string is correct (assuming you don't already have a comprehensive answer).

neurolabusc commented 1 year ago

@lukeje please also take a look at the validation series DKI_b2000_20_2p2mm_iPAT where the CSA slice timing clearly demonstrates a multi-band of two.

This is clearly noted in the tag 0051,1011:

(0018,1020) LO [syngo MR E11]            #SoftwareVersions
(0018,1030) LO [DKI_b2000_20_2p2mm_iPAT] #ProtocolName
(0051,1011) LO [p2 s2]                   #Private Tag & Data

However, the explicit SMS tag in the CSA header incorrectly suggests that the MultibandAccelerationFactor is 1:

sPat.lAccelFactPE    =  2
sPat.lAccelFact3D    =  1

This dataset appears to provide the opposite interpretation to yours, suggesting that precedence should be given to 0051,1011. Perhaps this reflects the custom CMRR sequences (ConsistencyInfo: N4_VE11C_LATEST_20160120), but it would be great to have an algorithm that works reliably for both product and research sequences.

neurolabusc commented 1 year ago

@lukeje perhaps the issue here is the nomenclature for SMS acceleration of 2D EPI scans and 3D sequences. The BIDS specification clearly defines MultibandAccelerationFactor for EPI scans, but does not (yet) define ParallelReductionFactorOutOfPlane that would be an analog of DICOM 0018,9155. dcm2niix already defines a lot of DICOM tags that are not in the BIDS standard, so I am happy to add this while we lobby BIDS to include this one-to-one mapping.

The main thing I would want is some clarity regarding when we use the field MultibandAccelerationFactor and when we use ParallelReductionFactorOutOfPlane, along with validation datasets.

@mharms may have some thoughts on this.

lukeje commented 1 year ago

Sorry, I thought you knew that ParallelReductionFactorOutOfPlane is in the latest BIDS specification (see discusion here).

neurolabusc commented 1 year ago

The creators of the Example dataset for the hMRI toolbox should be admonished, as this provides an example of desired output without a reference input dataset. I am glad BIDS now explicitly encourages sourcedata files. There has been a rampant creation of new BIDS fields that represent a wish list of desired sequence information with no consideration of how the user will determine this information, describing how different manufacturers use these terms, and validation datasets for regression detection. The opportunity for unintended consequences is profound. I worry that new BIDS fields are created by experts in sub-domains, but the burden of implementing them is pushed to users who do not have the expertise to decipher what is desired.

Perhaps your earlier link to a DICOM dataset is an attempt to rectify this situation.

If you want these fields to be supported, I strongly encourage following the format of the dcm_qa series that provides explicit examples for usage such as slice timing, enhanced DICOM, and gantry tilt, and ASL parameters. These provide a one-to-one mapping from source DICOM images to expected BIDS results. Each commit of dcm2niix is tested with a battery of these validation datasets to avoid regressions, and this allows us to harmonize our use with other tools such as dicm2nii. The other advantage of these datasets is that they help identify gaps in the current DICOM specification, which allows us to work with manufacturers and the DICOM work groups to aid reproducible science.

mharms commented 1 year ago

@lukeje please also take a look at the validation series DKI_b2000_20_2p2mm_iPAT where the CSA slice timing clearly demonstrates a multi-band of two.

This is clearly noted in the tag 0051,1011:

(0018,1020) LO [syngo MR E11]            #SoftwareVersions
(0018,1030) LO [DKI_b2000_20_2p2mm_iPAT] #ProtocolName
(0051,1011) LO [p2 s2]                   #Private Tag & Data

However, the explicit SMS tag in the CSA header incorrectly suggests that the MultibandAccelerationFactor is 1:

sPat.lAccelFactPE  =  2
sPat.lAccelFact3D  =  1

This dataset appears to provide the opposite interpretation to yours, suggesting that precedence should be given to 0051,1011. Perhaps this reflects the custom CMRR sequences (ConsistencyInfo: N4_VE11C_LATEST_20160120), but it would be great to have an algorithm that works reliably for both product and research sequences.

@neurolabusc : I think there is a confusion in your earlier post. I'm pretty sure that sPat.lAccelFact3D represents what is being called ParallelReductionFactorOutOfPlane, not the multi-band factor. So, the validation series you were referencing would have GRAPPA = 2 x 1, with MB factor = 2.

neurolabusc commented 1 year ago

@mharms yes, I realized that when I wrote my comment regarding 2D vs 3D data. So the s value of (0051,1011) encodes the product of sPat.lAccelFactPE and sPat.lAccelFact3D. This would provide some rationale for giving precedence to the CSA header when it is available.

This suggests we extend the V* conversion:

To be comfortable adding support for this, I would really like to have a concrete example where ParallelReductionFactorOutOfPlane and ParallelReductionFactorInPlane are modulated to document and replicate this interpretation. Since support for Windows 7 has now ended, it would be great to see concrete examples for V*, XA interoperability/classic and XA enhanced.

mharms commented 1 year ago

Isn't it the p (not s) value of (0051,1011) that is giving the product of the two GRAPPA accelerations?

So, if I'm inferring correctly, ParallelReductionFactorInPlane is currently taken from the p value of (0051,1011), not from sPat.lAccelFactPE ?

neurolabusc commented 1 year ago

@mharms, yes. And if we are lucky with XA ParallelReductionFactorOutOfPlane is reported by DICOM Tag 0018, 9155.

mharms commented 1 year ago

I see that it's reported in some structural scans I have as Enhanced DICOMs from XA30:

    (0018,9069) FD 2                                        #   8, 1 ParallelReductionFactorInPlane
    (0018,9077) CS [YES]                                    #   4, 1 ParallelAcquisition
    (0018,9078) CS [GRAPPA]                                 #   6, 1 ParallelAcquisitionTechnique
    (0018,9079) FD 1000                                     #   8, 1 InversionTimes
    (0018,9081) CS [NO]                                     #   2, 1 PartialFourier
    (0018,9155) FD 1                                        #   8, 1 ParallelReductionFactorOutOfPlane

None of these (0018,9***) field are included in Classic DICOMs exported from the same scanner.

I don't know if it adds anything, but there is a sPat.dTotalAccelFact field, which I assume is just the product of sPat.lAccelFactPE and sPat.lAccelFact3D.

lukeje commented 1 year ago

Hey Chris, I would appreciate it if you stopped tilting at windmills. I have made a targeted bug report and would appreciate it if you remained on topic. As has become clear, the current behaviour of dcm2niix does not match what would be expected.

You are correct that the dataset I am currently converting is related to me trying to prepare a dataset for you in relation to a previous bug report. However the extra fields that were requested there are not relevant here. Indeed, I hope that we (and other members of the hMRI toolbox development team) can work together to include the extra fields from the qMRI "wishlist" in future, after further communication.

neurolabusc commented 1 year ago

@lukeje I only have a single exemplar, so I would be grateful if can you test out the latest commit to the development branch (v1.0.20230127). It uses the new BIDS tag ParallelReductionFactorOutOfPlane instead of the previous AccelFact3D (if from the CSA) or ParallelReductionOutOfPlane (if from 0018, 9155 or 0043,1083). The CSA values are given precedence over (0051,1011).

You can get a pre-compiled copy by selecting the desired operating system and then artifact from AppVeyor, or build it yourself with UNIX:

git clone --branch development https://github.com/rordenlab/dcm2niix.git
cd dcm2niix/console
make
neurolabusc commented 1 year ago

@mr-jaemin can you please test the behavior of the latest commit. I assume that I use the reciprocal of the second item in the 0043,1083 for ParallelReductionFactorOutOfPlane. This is related to your help with issue 641.

The exemplar I have is the T1 scan from dcm_qa_ge which reports

(0043,1083) DS [0.5\0.5]

which I assume converts to BIDS:

"ParallelReductionFactorInPlane": 2,
"ParallelReductionFactorOutOfPlane": 2,
mr-jaemin commented 1 year ago

I collected different combinations of ParallelReductionFactorInPlane (Acceleration-> Phase) ParallelReductionFactorOutOfPlane (Acceleration-> Slice) on GE as shown below Screen Shot 2023-02-01 at 9 20 03 AM

For completeness, I also collected a dataset with Acceleration -> HyperSense (Compressed Sensing) (although this is off the topic for this particular issue) Screen Shot 2023-02-01 at 9 27 05 AM

Here is the list of scans, where P: Phase, S: Slice, H: HyperSense Screen Shot 2023-02-01 at 9 40 01 AM

I confirmed that everything is working as expected. For example, 09_T1_MPRAGE_P1.2S1.44 reports

(0043,1083) DS [0.833333\0.694444]                      #  18, 2 AssetRFactors

which converts to BIDS

    "ParallelReductionFactorInPlane": 1.2,
    "ParallelReductionFactorOutOfPlane": 1.44,

The stable release of dcm2niix (v1.0.20220915), as expected, reports Acceleration-> Slice as ParallelReductionOutOfPlane, not ParallelReductionFactorOutOfPlane

    "ParallelReductionFactorInPlane": 1.2,
    "ParallelReductionOutOfPlane": 1.44,

Perhaps, I will discuss with Chris separately about HyperSense's Acceleration factor (as shown below) and how to share these exemplars publicly.

(0043,10b7) LO [1.24\1\10\0]                            #  12, 4 Compressed Sensing Parameters
neurolabusc commented 1 year ago

@mr-jaemin thanks. Closing this issue as it passes all samples provided. I will deal with compressed sensing and deep learning in a future issue when I have samples from other vendors.

lukeje commented 1 year ago

I would be grateful if can you test out the latest commit to the development branch (v1.0.20230127).

Sorry for the late reply; yes, the new development branch works as expected.

Thanks a lot!