Dev fix TotalReadoutTime for GE with interpolation

[brice82]

This is a fix for correcting TotalReadoutTime and EffectiveEchoSpacing in the BIDS (.json) sidecar in case of image interpolation.

TotalReadoutTime and EffectiveEchoSpacing were correct when there was no interpolation (i.e. AcquisitionMatrixPE = ReconMatrixPE). However, when interpolation was used, this values were incorrect. When interpolation was used, the estimated fieldmap given by TOPUP was ReconMatrixPE/AcquisitionMatrixPE times a measured fieldmap. Also, the same interpolated images corrected with a B0Map based method via FSL's epi_reg were clearly under-corrected.
It turned out that the problem was coming from TotalReadoutTime and EffectiveEchoSpacing which were not taking ReconMatrixPE correctly into account.
This is a Fix for that.

GE/README.md was modified to clarified how TotalReadoutTime and EffectiveEchoSpacing are now computed.
Some intermediate variable were also introduced and described. This should help the final user to better understand.

console/nii_dicom_batch.cpp was modified to implement the changes on TotalReadoutTime and EffectiveEchoSpacing.
The intermediate variables described in README were also added into the json BIDS sidecar (i.e. EchoSpacingMicroSecondsGE, NotPhysicalNumberOfAcquiredPELinesGE, NotPhysicalTotalReadOutTimeGE). As the TotalReadoutTime definition is not intuitive this should help the user to understand what is going on.

On branch dev-fix-TotalReadoutTimeGE-with-interpolation Changes to be committed:
modified: GE/README.md
modified: console/nii_dicom_batch.cpp

[neurolabusc]

Thanks!

[mharms]

Couple comments/thoughts:

1. The definition of NotPhysicalTotalReadOutTimeGE in the revised README references a variable, EchoSpacing which itself does not appear to be referenced/defined in the README section for Total Readout Time.
2. I think that adding these variables to the json (EchoSpacingMicroSecondsGE, NotPhysicalNumberOfAcquiredPELinesGE, NotPhysicalTotalReadOutTimeGE) may actually lead to more ambiguity/confusion than clarity. If the equations in the README are accurate, and the necessary input variables are all available in the json, I don't know if it is advantageous to include what are effectively just intermediate variables in a calculation.

[neurolabusc]

@mharms the main improvement of the PR is to provide the FSL/BIDS definition of TotalReadoutTime for situations including interpolation. I think we can discuss if the additional non-BIDS fields should be included are a valid issue for discussion prior to the next stable release. For completeness, there is also a validation dataset.

[mharms]

If I'm reading the PR right, it itself has already added 3 new variables to the json.

[brice82]

Thanks.
Yes, the aim was provide the FSL/BIDS definition of TotalReadoutTime and EffectiveEchoSpacing for situations including interpolation.
If you think the intermediate variables (EchoSpacingMicroSecondsGE, NotPhysicalNumberOfAcquiredPELinesGE, NotPhysicalTotalReadOutTimeGE) does not help, this is no problem to remove them.

@mharms EchoSpacing is the "Effective Echo Spacing (0043,102C)" from GE DICOMS and is defined as the time between two consecutive acquired phase encoding lines divided by the number of shots.
I agree that this should be clarified to avoid any ambiguity. I'll do it.

[mharms]

Personally, I don't think writing those 3 variables into the json is helpful. I'm not sure where @neurolabusc thinks that should be resolved however, other than right here, at this time.

FYI, There is a typo in the following if you want to fix:

Please, note that these two intermadiate variables does not take partial Fourier into account.

It should read:
Please, note that these two intermediate variables do not take partial Fourier into account.

[neurolabusc]

@brice82 since you agree with @mharms, can I suggest you make a pull request that does not save these intermediate values by default, but will generate them if re-compiled in debug mode:

#ifdef MY_DEBUG
		fprintf(fp, "\t\"EchoSpacingMicroSecondsGE\": %d,\n", d.effectiveEchoSpacingGE);
		fprintf(fp, "\t\"NotPhysicalNumberOfAcquiredPELinesGE\": %d,\n", (int)(NotPhysicalNumberOfAcquiredPELinesGE));
		json_Float(fp, "\t\"NotPhysicalTotalReadOutTimeGE\": %g,\n", NotPhysicalTotalReadOutTimeGE);
#endif

[brice82]

No problem at all. Anyway, this can be sorted out thru the official BIDS field and the formulas in GE/README.md
@mharms Thanks for spotting it. I'll correct it.
@neurolabusc I will add the the intermediate variables in debug mode only. This makes sense.
Thanks

[brice82]

submitted PR #730