Support for GE HealthCare (GEHC) B0 Fieldmap processing.

[brice82]

Correction of the existing GEHC B0 fieldmap processing (which was incomplete) and implementation of a new B0 fieldmap processing for GEHC data. The minimum required FSL-version is 6.0.7.1 for GEHC B0 fieldmap processing.

The existing fieldmap processing "GeneralElectricFieldMap" was renamed "GEHealthCareLegacyFieldMap". It is made for Legacy existing data converted with dcm2niix (pre-v1.0.20210410). These GEHC B0Maps were a 2 volumes NIFTI file (Volume 1 is the B0Map in Hertz, and Volume 2 the magnitude image).

A new fieldmap processing is introduced and named "GEHealthCareFieldMap". This is intended for new data converted with recent dcm2niix (v1.0.20220720 and later). The GEHC B0Maps are split as 2 NIfTI files (the magnitude and the b0map_fieldmaphz in Hertz - with the suffix "_fieldmaphz" for easy identification). The usage of this fieldmap processing is identical to Siemens/Philips pipeline.

For both "GEHealthCareLegacyFieldMap" and "GEHealthCareFieldMap" correction methods the "DeltaTE" is now a mandatory input.

Description of changes:
modified: global/scripts/fsl_version.shlib
Functions to format FSL-version strings, to compare FSL-version and to check that minimun required FSL-version were added.

modified: global/scripts/FieldMapPreprocessingAll.sh
Modification of "GeneralElectricFieldMap" renamed "GEHealthCareLegacyFieldMap" to add phase unwrapping and processing with fsl_prepare_fieldmap.
Added the "GEHealthCareFieldMap" processing via fsl_prepare_fieldmap.
The input DeltaTE is now mandatory.
And a verification that the minimum FSL-version criterion is met.

The following scripts were also modified to reflect the changes made in global/scripts/fsl_version.shlib and mainly global/scripts/FieldMapPreprocessingAll.sh.

modified: Examples/Scripts/GenericfMRIVolumeProcessingPipelineBatch.sh
modified: Examples/Scripts/Historical/7T/GenericfMRIVolumeProcessingPipelineBatch.7T.sh
modified: Examples/Scripts/PreFreeSurferPipelineBatch.sh
modified: PreFreeSurfer/PreFreeSurferPipeline.sh
modified: PreFreeSurfer/scripts/T2wToT1wDistortionCorrectAndReg.sh
modified: fMRIVolume/GenericfMRIVolumeProcessingPipeline.sh
modified: fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh

Modification tested on a database of 7 subjects containing 3DT1w, 3DT2w, resting-state fMRI, 3D-B0maps, and a pair of Spin-Echo EPI with positve and negative phase-encodings (a.k.a SpinEchoFielMaps for TOPUP).

[glasserm]

I would like to seem some fMRI test results. Also, why did you modify the Philips section to remove the DeltaTE requirement?

[brice82]

I will sent you the fMRI data (tomorrow). I used the Topup corrected images as a reference, and the B0map corrected fMRI data are very close to the Topup corrected fMRI data.

=> "why did you modify the Philips section to remove the DeltaTE requirement ?"
Simply because DeltaTE is now a Mandatory option in every case (Philips, Siemens, GEHC).
So I have removed from the Philips only and Siemens only sections.
(The DeltaTE changed from optionnal to mandatory, and it is now tested as such, so no need to retest for specific cases after. )

[glasserm]

Okay thanks.

[glasserm]

I have reviewed some test data from Brice and it looks good. I am okay to merge if others are.

[coalsont]

Logic looks reasonable. I have made some documentation/formatting/typo suggestions.

[brice82]

The latest commits should address the comments from @mharms.
In Summary:

• GENERAL_ELECTRIC_METHOD_OPT has been renamed GE_HEALTHCARE_LEGACY_METHOD_OPT
• Removed the TE / DeltaTE confusion in PreFreeSurferPipelineBatch.sh (it is now DeltaTE)
• option --fmapgeneralelectric is now just --fmap (only used in the GE_HEALTHCARE_LEGACY_METHOD_OPT) and all other field map methods uses --fmapmag and --fmapphase
• copy a number of Mike's corrections from GenericfMRIVolumeProcessingPipelineBatch.sh to GenericfMRIVolumeProcessingPipelineBatch.7T.sh

I left the duplication of checking FSL version for GEHC in GenericfMRIVolumeProcessingPipeline.sh.
The idea is to throw the error before starting any processing, right at the beginning. This should make the end-user experience easier.
Let me know if you want this FSL version check removed.

Thanks @mharms for the numerous typos correction and documentation clarification.

[mharms]

I would suggest we name the flag --fmapgelegacy instead, to make it clear in the flag name that it applies to a fairly specific use case.

[mharms]

Also, if @coalsont wants to keep the FSL version in two different places, please at least add a comment at that point in the code in GenericfMRIVolumeProcessingPipeline.sh that the check is duplicated in the lower level field map processing script (whatever it is called), so that if we ever need to change it, we are reminded of the 2nd location.

[brice82]

Done (both --fmapgelegacy and added a comment for the duplicated FSL-Version check).

@coalsont Let us know what you prefer for the FSL-Version check ?
It is in FieldMapProcessingAll.sh and one more GenericfMRIVolumeProcessingPipeline.sh, The intention is to catch the error as early as possible.

Another possibility is to move the minimun version declaration (also duplicated)

# Minimum required FSL version for GE_HEALTHCARE_LEGACY_METHOD_OPT and GE_HEALTHCARE_METHOD_OPT
GEHEALTHCARE_MINIMUM_FSL_VERSION="6.0.7.1"

into global/scripts/fsl_version.shlib
Then the the minimum version for GE Fieldmap support will be define only once.

[coalsont]

Yes, erroring early is better, so having the check in two places is fine.

I think the --fmap flag should be simply "use this if the magnitude and phase are concatenated in a single file", I actually don't think it is correct to rename it based on the mere fact that only one method currently uses that format. Let's not be excessively specific in option names such that it would make them awkward to use them for a similar purpose with a different brand. If we wanted to be more specific in a brand-agnostic way, we could call it --fmapcombined or similar.

[mharms]

In that case, I'd favor --fmapcombined

[mharms]

More precisely, make --fmapcombined the forward facing flag, but I'm fine with continuing to support --fmap as an alternative, given that the parsing library makes that easy to do.

[brice82]

To finalize this PR and address the remaining comments from @mharms and @coalsont, I did:

• Use --fmapcombined as suggested (and --fmap as hidden flag). So there is no more compagny names in the command-line.
• Moved the definition GEHEALTHCARE_MINIMUM_FSL_VERSION in global/scripts/fsl_version.shlib as a global constant (now it is defined only once) and added appropriate comments. This should ease the maintenance.

I feel like we should be good to go now.
Let me know.

[mharms]

I don't have anything else to add, other than thanks @brice82 for these changes and additions. This is a great example of a community contribution!

[brice82]

I converted all tabs into 4 spaces in GenericfMRIVolumeProcessingPipelineBatch.7T.sh and GenericfMRIVolumeProcessingPipeline.sh and also fix the indentation, there were plenty of tabs remaining.
Difference should then be whitespace only.

Thanks a lot @mharms and @coalsont .

This time the PR should be good to go.

[coalsont]

That's one way to do it. I don't think we cared about "git blame" on the batch file, and it looks like a fair amount of fMRIVolume wasn't using tabs, so most of its "git blame" will survive.

Did you test it after changing --fmap to --fmapcombined? I think I'm happy now (sorry for not catching all the formatting in the first review).

[brice82]

Yes, I rerun all the test scripts (from PreSurfer until fMRIVolume) with --fmapcombined and --fmap but on a single subject.
Same output. No error.
Run test script of FieldMapProccessingAll.sh with --fmap / --fmapcombined (with legacy pipeline) and with --fmapphase and --fmapmag (New processing pipeline) on the 7 subjects.
Identical results. No error.
I concluded that everything was as expected.

PS: Tabs should be forbidden by law :-)

[glasserm]

Do folks feel this is ready to merge?