No more slice removal for odd number of slices

[MichielCottaar]

This is meant to address #217 . This pull request is still in draft, because the topup will crash for any images with odd number of slices. We still need to adjust the topup config file used (either for all input volumes or only for those with odd number of slices).

[glasserm]

Thanks Michiel. Is this tested? It looks good from a code skim.

[MichielCottaar]

I'm running some overnight tests on HCP YA data. I'll let you know tomorrow

[MichielCottaar]

This is working, I've tested it in three specific cases. In all cases the input was the HCP YA adult data, which has an odd number of slices.

1. The default pipeline without any additional flags fails in that case, because the default topup configuration file is incompatible with an odd number of slices. I've copied the error message raised by topup below.
2. When adding the --ensure-even-slices flag, the pipeline removed a slice from both the input to topup and eddy. Topup ran succesfully afterward. I've not tested the rest of the pipeline, but see no reason why it would not work. This matches the original behaviour and will be incompatible with slice-to-volume correction in eddy.
3. When adding the --topup-config-file=<file> flag with a topup configuration without subsampling, topup also ran succesfully (without removing any slices).

The error message raised by topup if the subsampling levels are incompatible with the image dimensions

Part of FSL (ID: 6.0.6:e380285d)
topup

Usage: 
topup --imain=<some 4D image> --datain=<text file> --config=<text file with parameters> --out=my_topup_results


Compulsory arguments (You MUST set one or more of):
	--imain		name of 4D file with images
	--datain	name of text file with PE directions/times

Optional arguments (You may optionally specify one or more of):
	--out		base-name of output files (spline coefficients (Hz) and movement parameters)
	--fout		name of image file with field (Hz)
	--iout		name of 4D image file with unwarped images
	--logout	Name of log-file
	--warpres	(approximate) resolution (in mm) of warp basis for the different sub-sampling levels, default 10
	--subsamp	sub-sampling scheme, default 1
	--fwhm		FWHM (in mm) of gaussian smoothing kernel, default 8
	--config	Name of config file specifying command line arguments
	--miter		Max # of non-linear iterations, default 5
	--lambda	Weight of regularisation, default depending on --ssqlambda and --regmod switches. See user documetation.
	--ssqlambda	If set (=1), lambda is weighted by current ssq, default 1
	--regmod	Model for regularisation of warp-field [membrane_energy bending_energy], default bending_energy
	--estmov	Estimate movements if set, default 1 (true)
	--minmet	Minimisation method 0=Levenberg-Marquardt, 1=Scaled Conjugate Gradient, default 0 (LM)
	--splineorder	Order of spline, 2->Qadratic spline, 3->Cubic spline. Default=3
	--numprec	Precision for representing Hessian, double or float. Default double
	--interp	Image interpolation model, linear or spline. Default spline
	--scale		If set (=1), the images are individually scaled to a common mean, default 0 (false)
	--regrid		If set (=1), the calculations are done in a different grid, default 1 (true)
	-h,--help	display help info
	-v,--verbose	Print diagonostic information while running
	-h,--help	display help info



Topup: msg=topup_clp::topup_clp: Subsampling levels incompatible with image data.
Mon Jan 10 14:40:58 GMT 2022:run_topup.sh: While running '/home/fs0/ndcn0236/Work/datalad/HCP_test_slice_removal/Pipelines/DiffusionPreprocessing/scripts/run_topup.sh /home/fs0/ndcn0236/Work/datalad/HCP_test_slice_removal/base/103818/Diffusion/topup /home/fs0/ndcn0236/Work/datalad/HCP_test_slice_removal/Pipelines/global/config/b02b0.cnf':
Mon Jan 10 14:40:58 GMT 2022:run_topup.sh: ERROR: '/opt/fmrib/fsl/bin/topup' command failed with return code: 1

[glasserm]

This is nearly there. I do think the topup error message is actually not as userfriendly as we would like. It would be better to catch the error early in the pipeline (before much processing has taken place) and to suggest the user to 1) Use a different config file (recommended) or 2) Use the --ensure-even-slices flag (if trying to replicate legacy behavior or if not using advanced eddy features). Does that seem reasonable to you?

[MichielCottaar]

Sounds reasonable. I thought that I might need to parse the topup config file to do that. However, I just realised that we could simply raise an error if (1) there is an odd number of slices and (2) there is no --topup-config-file or --ensure-even-slices flag. I'll add an error in that case.

That would mean that if the user provides a topup config file that is incompatible with the data dimensions, they would still get the topup error. Is that okay with you?

[glasserm]

Is parsing the file hard? @coalsont might be able to help with the code for this. Basically it needs to grep the line:
--subsamp=2,2,2,1,1 from the planned topup config and then check whether any digits are not 1.

[MichielCottaar]

It would take me quite a while to code that up in bash (I'm not very good in bash, grep, etc.). If someone can give me the code, I'll be happy to put it in.

[glasserm]

@coalsont is better than me, but I think it will be a one-liner. Something like testing whether cat <config> | grep "--subsamp=" | sed 's/--subsamp=//g' | sed 's/,//g' | sed 's/1//g' returns a non-blank string.

[coalsont]

Honestly, if the user supplies their own topup config, I think they can deal with the errors they get from topup themselves, so I don't know that I would try to parse it to give an error in advance (maybe a future topup feature will allow subsampling without requirements on dimensions, which could make our error on someone else's config wrong and annoying). I would recommend an option to ignore this check if we want our script to check custom config files by default.

If you want to try to protect users from their own custom config "mistakes" anyway, sed 's/,//g' is a bit brutal, and theoretically wouldn't catch "11", though hopefully nobody would use that. Something like | grep -Evq '^(1,)*1$' on the end instead of the comma, 1 pair of seds would give an exit status indication you could use in a conditional:

if grep -e "--subsamp=" <config> | sed 's/^.*--subsamp=//g' | grep -Evq '^[:space:]*(1,)*1[:space:]$' && [[ <slice count is odd> && "$EnsureEvenSlices" != "true" ]]
then
    log_Err_Abort "topup config uses subsampling, use --ensure-even-slices"
fi

If topup configs forbid whitespace, then some of the extra stuff in the regexps can be removed.

Technically, this assumes that the only kind of subsampling that will ever be in anyone's config is 2, since it assumes that an even number makes anything fine. The current code also assumes that only the slice direction can be an issue. Not sure how deep into this rabbit hole we really want to go, as long as we give the user the option of "just try it and see what topup thinks". Really, the right solution is to ask FSL to improve topup's error messages.

[mharms]

Given that a 4x subsampling config file is now an option that FSL provides, and that the proposed check is only on the slice dimension, maybe it is indeed best to not get into the checking business at all (given that what we are proposing is not a complete check for the ways that the config file can be incorrect relative to the image dimensions).

But I defer to the consensus of others.

[glasserm]

The topup error doesn't really tell the user how to resolve the problem as far as the HCP Pipelines are concerned. If we wanted to catch and parse the topup error, that would be another option...

[coalsont]

Detecting that topup crashed with a custom config and saying "if your config contains subsampling, make sure your image dimensions are an exact multiple" wouldn't require parsing anything. When using the pipelines' own config files, we can provide more accurate errors in advance (because we know what they require), again without parsing anything.

[mharms]

Looks good. Thanks @MichielCottaar. I've suggested some minor tweaks to some of the messages.

[MichielCottaar]

I agree that the error messages from topup could be improved, but this does not seem to be the place to do so. Messages like "if your config contains subsampling, make sure your image dimensions are an exact multiple", would be much more useful in FSL's topup itself (there is nothing particular to the HCP pipelines about this).

I agree with @coalsont of checking the data dimensions in advance if using the HCP pipeline's config file (as we now do), but we should just leave it to topup if the user is confident enough to provide their own config file.

[glasserm]

Ok

[mharms]

So, that does mean we are basically done, pending any further requests/suggestions from either @coalsont or @glasserm ?

[glasserm]

Well, I'd like things to be as userfriendly as possible, but don't want to hold things up if there isn't consensus on that.

[glasserm]

Shall we merge this so that we can tag a release once the tICA testing is done?

[mharms]

That's fine with me.