Feature request: dwipreproc to include dwidenoise and mrdegibbs option

[neurolabusc]

I realize that dwipreproc already does a lot, but it would be handy if the user could optionally request dwidenoise and mrdegibbs. I find dwipreproc very handy in that it removes temporary files, and deals with file renaming.

[jdtournier]

This relates to a discussion we had not too long ago, about potentially renaming dwipreproc to better reflect the fact that there were other preprocessing commands that many/most users might potentially want to run as well. At the time, we'd only discussed renaming dwipreproc, but somehow no-one suggested including dwidenoise and mrdegibbs within dwipreproc... I reckon it's a good suggestion, and would actually address the issues raised in that original thread better than any renaming of dwipreproc would... What do you guys think?

[Lestropie]

• This would not really be consistent with the design philosophy of MRtrix3, where we have individual tools with clear purposes.

• There are some data for which dwidenoise and mrdegibbs are not applicable.

• dwipreproc is already the primary cause of loss of sleep for me; would prefer to not complicate it even further :-/

Personally I think @jdtournier's link regarding the naming of dwipreproc (also likely to be ongoing in #1147) is highly pertinent, but draw a different conclusion from it. For example: If the dwipreproc script were in fact named dwiundistort, it would not seem appropriate to incorporate dwidenoise and mrdegibbs into a script with that name. dwipreproc was only called "preproc" because it was the only "compulsory" stage of pre-processing at the time; but that is no longer the case.

What would make more sense IMO, would be to have another script (whether developed yourself or part of MRtrix3), possibly even called dwipreproc if the current dwipreproc were renamed, that performed "all" DWI pre-processing. So that would be denoising, Gibbs ringing removal, topup / eddy, B1 bias field correction, maybe also image FoV cropping. So basically this block in the MRtrix3_connectome script. If this were developed against the MRtrix3 Python libraries, you'd still get the benefits of temporary file removal, consistent terminal I/O etc..

This approach would mean that anybody wanting to replace / remove the fundamental elements of DWI pre-processing would still have free access to the individual tools, and would be able to try modifications to the overall DWI pre-processing wrapper script (as opposed to the current dwipreproc, which even I struggle wrapping my head around to make modifications, and I wrote the damned thing).

In my BIDS app, the file naming & presence of necessary JSON fields for phase-encoding information is used to greatly simply the interface, essentially removing the need for the user to specify the nature of their data. My experience would suggest that a similar restriction should apply to such a script in MRtrix3 also; not necessarily enforcing a BIDS input, but enforcing that the requisite phase-encoding key-value fields are preserved in the image headers from DICOM import to execution of the script. So for instance, the user would concatenate all DWI series into a single image, concatenate all reversed phase-encode spin-echo EPI series to be used for inhomogeneity estimation into a single image, and then pass just those two images to the script. The script currently known as dwipreproc would then be executed within this higher-order DWI pre-processing script using its -rpe_header option. Without imposing this limitation, such a script would inherit the complexity of the current dwipreproc interface, and therefore it wouldn't actually serve all that much of a purpose.

[thijsdhollander]

This would not really be consistent with the design philosophy of MRtrix3, where we have individual tools with clear purposes. There are some data for which dwidenoise and mrdegibbs are not applicable. dwipreproc is already the primary cause of loss of sleep for me; would prefer to not complicate it even further :-/ Personally I think @jdtournier's link regarding the naming of dwipreproc (also likely to be ongoing in #1147) is highly pertinent, but draw a different conclusion from it. For example: If the dwipreproc script were in fact named dwiundistort, it would not seem appropriate to incorporate dwidenoise and mrdegibbs into a script with that name. dwipreproc was only called "preproc" because it was the only "compulsory" stage of pre-processing at the time; but that is no longer the case.

Yeah, I fully agree with this. The "dwipreproc" name is really only a historical remnant of what "preprocessing" used to mean and be. Currently, in a manually performed FBA or connectome analysis, the separate preprocessing steps (before response function estimation and CSD) easily become a third of the steps, and a third of the QA time better spent consciously looking at images to make sure things go well and assumptions apply (see all recent (internal) discussions on dwidenoise and mrdegibbs and topup/eddy combinations and bias field correction as excellent examples of all of this being non-trivial). I'm sitting right next to RA's and collaborators a lot of my time related to some/any of these steps, so I'm first-hand aware of the pain that these can cause if not treated with the full separate attention they deserve. And it's never fun to have to re-run computationally expensive steps such as 3-tissue CSD, template building and fixelstats, just because a mistake happened during the preprocessing that preceded those.

I also picked up that there was (again, historically, but close to all good now) some "unhappiness" about it not being clear enough that dwipreproc was essentially FSL^2 and not much (if at all) MRtrix. This is all better now due to clear and explicit inclusion of references and acknowledgements all over the place (to the benefit of both the relevant FSL developers and ourselves). But this would definitely not become easier by boxing up so many different steps. I know a script can easily be pulled apart by people like us, but many users take a box as a box: something that comes as one, and is not easily broken up. With the risk that a suboptimal outcome is all too easily blamed on the wrong bit of the box. Again, a recipe for unhappy developers of any particular steps included in the box.

I'm myself a bit wary to pack anything that combines multiple steps together with MRtrix3 directly. If people just want to automate stuff for a particular (range of) study/ies, it's easy enough to write those 4 lines of MRtrix preprocessing commands/scripts once in a batch file (our RA's do that successfully without loss of efficiency), and just run that; or indeed use facilities suited specifically for such purposes, such as BIDS apps or similar containerised solutions. The current manual already allows more or less copy-pasting of those 4 lines, but at least the users will have then seen those lines and hopefully thought about their applicability to their data.

So well, long rant... :grin: ...but I'm still heavily in favour of a rename to dwiundistort. :man_shrugging:

[Lestropie]

Bumping due to milestone inclusion: I'd give a +1 to a rename to dwiundistort. Or indeed even dwifslundistort if it would reduce risk of future confusion with Daan's contributions / better communicate where the heavy lifting is done; hell of a mouthful though, would immediately need the nickname "undistort" for verbal communication.

Not really up for writing a script to combine multiple pre-processing steps, as I can simply use my BIDS App for that purpose, and that's where my efforts in "automating multiple processing steps" will be going.

[thijsdhollander]

For the record: I still support this heavily as well!

About the nuances: what about dwiundistort with the functionality of algorithm selection? So (for now) that would then essentially be dwiundistort fsl; furthermore neatly in line with 5ttgen fsl and dwibiascorrect fsl. Minor added benefit: makes "fsl" stand out better (even if only because of the space).

[Lestropie]

Trouble with that approach is that if novel DWI distortion correction is introduced into MRtrix3, I'd have expected it to be entirely in C++. So to have both "algorithms" going through a single interface, the "MRtrix3 algorithm for the dwiundistort Python script" would need to invoke binary MRtrix3 commands to serve the purpose of DWI distortion correction; but what would the names of those commands then be? I suspect that for the "principle" command of such an addition, dwiundistort would be an entirely appropriate name... @jdtournier @dchristiaens ?

[dchristiaens]

First of all, regarding the feature request in the title: I am not in favour of a single script to do denoising, Gibbs ringing removal, and distortion&motion correction. I agree with @Lestropie that such can be better done within the BIDS app. But more importantly, I also don't think it would be of much benefit to the users. These 3 steps are typically run sequentially, without overlap in their required parameters. A single, integrated preprocessing script would then have to expose the options of dwidenoise and mrdegibbs, leading to a long and messy command syntax. Personally, I prefer 3 readable command calls instead of a single one that gives me a headache. I expect users will write their own bash script anyways...

Regarding the naming of dwipreproc, I agree that this is a bit of a misnomer and we could come up with something better. I am very happy if we could find a name that links up with (or at least doesn't clash with) my current work on motion correction. @Lestropie My implementation uses a python script dwimotioncorrect that alternates between reconstruction, registration, and outlier rejection steps, each implemented in separate C++ binaries (dwirecon, dwislicealign, etc.). The field map is currently an input to the script, which I generate from Topup. I suppose we could make this a separate 'algorithm' in the way we also do for 5ttgen and others, to live next to a 'fsl' algorithm that chains up topup and eddy, although the output of my method is conceptually somewhat different from that of eddy.

[thijsdhollander]

Doesn't look like the original request is going to happen or be preferred by a majority of people, so I'll close this issue for now, but I'll create a fresh one for the dwipreproc rename discussion (with a more relevant title). I'll link to this one for the arguments already summed up here.