OK, so I've successfully tested this with the case reported by the user (all DWIs acquired with reversed phase encoding, but with multiple b=0 volumes), using both -rpe_all and -rpe_header options. The changes also remove some erroneous error messages about incorrect phase encoding tables, much like what used to happen before #375 / #774 with gradient tables. So looking for a reviewer to OK... any takers?

Hadn't had the time to properly "review", but it sounds like it's all good, so I'll ok for the sake of the fix. Minor thing maybe (but I don't think this should hold back the fix in any way):

Commands that stash the DW scheme in order to prevent warnings / errors down the line also need to clear the phase-encoding scheme for the same reason.

Any particular reason for not also stashing the PE scheme in a sort of analogous way to the DWI scheme? (or just no good reason to stash it?)

Other non-urgent thing now that I think of it: with some of those soon upcoming changes as part of "RC2" happening any way... isn't it maybe about time to reconsider the name of this script while we're still beta enough? The name used to make sense, but is starting to become more and more confusing for new users, as we now also have denoising, maybe soon also unringing, and bias field correction. So this script isn't really "the" one and only preprocessing thing any more. Since the introduction of denoising it isn't even the first step any more. So I'd be eventually maybe thinking of a more specific name that communicates better what this command does (something that refers to distortion and motion somehow).

Any particular reason for not also stashing the PE scheme in a sort of analogous way to the DWI scheme? (or just no good reason to stash it?)

Basically no good reason, weighed off against the awkwardness...

• It's slightly weird because if the phase encoding is fixed for all volumes, it's stored in PhaseEncodingDirection and TotalReadoutTime rather than pe_scheme; "stashing" that into prior_pe_scheme as a complete table may appear odd.

• Should the "stashing" occur at the end of dwipreproc (since the relevant distortions have been corrected), or at the same time as the DW scheme stashing?

• What about when dwipreproc does volume combination? Then each volume has come from two volumes with different phase encoding.

Come to think of it, I suppose 2 and 3 are still outstanding...

... isn't it maybe about time to reconsider the name of this script while we're still beta enough?

Not against it; though I'd maybe wait for release rather than an RC update, don't want to give people a reason to not update. dwiundistort?

^^

Don't actually effect dwipreproc (since everything gets lost through the NIfTI conversion for eddy), but figured they made sense anyway. mrmath erases DW and PE schemes if operating across images or the volume axis, and mrcalc erases only if either scheme is inconsistent between input images.

What about when dwipreproc does volume combination? Then each volume has come from two volumes with different phase encoding.

Ah yes indeed, I hadn't appreciated that. Yes, you're right in that case: there's not really a clear definition of stuff any more, so stashing wouldn't really help (if anything maybe only cause confusion).

Not against it; though I'd maybe wait for release rather than an RC update, don't want to give people a reason to not update. dwiundistort?

Not sure; I'd think if people can agree on a name, it's probably better to change sooner rather than later. dwiundistort is a good suggestion by the way. Optimally, we could squeeze "motion" in there too, but it would become too unwieldy. In a way, motion is distortion of the expected location of the data in the 4D space-time domain. So if anything, undistort is probably still the best proposal that embodies all aspects of the functionality of the script.

About the renaming of dwipreproc: I'm not convinced there's a good case for it. It would cause quite a lot of frustration given how central this command is going to be in most users' pipelines, so I wouldn't do it lightly. Also, while I appreciate that denoising, bias field correction, and Gibbs ringing removal will no doubt be recommend in any preprocessing, they are not as central as motion and distortion correction - each of them can be omitted without invalidating the results. I think it still makes sense for that command to be perceived as the requisite pre-processing for any DWI analysis, while the others remain optional - as long as they're well documented. Just my 2 cents...

I'm not too obsessed with its name, dwipreproc surely still works. But just contributing to the arguments: I'm not 100% sure I'd label motion/distortion correction as the most important in all scenarios. I agree denoising is a nice bonus (that I'd surely recommend wherever it applies), but in a way not essential. Unringing, on the other hand, is in principle extremely important to get quantitative aspects right, especially since ringing affects b=0 versus b=something-else in ways that reinforce each other, changing diffusivity even beyond what is physically possible. That, combined with the typical low resolutions, is really quite bad. And next, there's bias field correction as well, which doesn't only affect the quantitative aspect in our FD-reality, but also the performance of mask generation and response function selection. If motion is limited (up to the (sub-voxel) point where you could sometimes wonder whether correction makes things better), it doesn't affect these things that much actually. Not that I would ever recommend against doing it of course... but just to put things in perspective.

Anyway, bringing this up was more about thinking whether the script's name could be more specific and better reveal what it packs. What I think makes the current name a bit more annoying as time goes on, is the introduction of steps that should definitely precede dwipreproc (both denoising and unringing). I appreciate it's still about preprocessing of course, but the name really starts to communicate less and less (1) what it's about and (2) where it sits in a typical pipeline. I'm personally not sure that the name should communicate the "importance" of the step (or give such kinds of advice via its name at all). I would rather emphasise these aspects in manuals and example / suggested pipelines.

So in conclusion: not overly fussed about it, but I think it's worthwhile to at least start thinking about this a bit, with the future (and a future with a release) somewhere in mind.

Just reviewed the entire code change: as far as I can see, it all looks ok. The only bit I was unsure of is https://github.com/MRtrix3/mrtrix3/pull/1047/files#diff-71c549699e123065bef9ad55c9c01d3fR90 , because I hadn't ever seen an ellipsis in a catch statement like that before (there's another one in another file change part of the pull request as well that I'd spotted). Looked it up, and it seems to be all right, it seems the exception is still successfully being re-thrown in such a scenario as well.

I don't have an appropriate dataset to test an -rpe_all scenario (nor do I have experience with that scenario to know what to reasonably expect as to performance from the recombination), but the code seems to do sensible steps, and take into account certain particular scenarios (e.g. gradient directions with opposing polarity) consciously. So I'm entirely happy for this to go into the upcoming tag branch, and be ready to go in master soon.

I propose we leave the name debate for now; it's a separate thing to the original intentions here any way. I've got the feeling we're not all going to be happy with a name change, or certainly not now already. I'm ok with that. 👍

mrmath seems to segfault during some mrfilter tests though, as well as during some tensor2metric tests.

This is an example bit of where it goes wrong:

mrmath: preloading data for "/tmp/mrtrix-tmp-b53ZJ7.mif"...  [==================================================]

/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/safe_iterator.h:279:

Error: attempt to dereference a past-the-end iterator.

Objects involved in the operation:

    iterator "this" @ 0x0x8ca790 {

      type = 

mrmath: [SYSTEM FATAL CODE: SIGSEGV (11)] Segmentation fault: Invalid memory access

I'm definitely not sure about this, but my guess would be that it has to do with this line: https://github.com/MRtrix3/mrtrix3/pull/1047/files#diff-8bd7800b444305a9a0153178f4a7ce15R352

I hadn't ever seen an ellipsis in a catch statement like that before

That's the catch-all statement - perfectly legit C++. We tend not to use it and rely on catching our own exceptions, since anything unexpected should really not be unexpected - all possible errors should be handled by the code responsible, and thrown as a dedicated MR::Exception if the application can't recover. So by only catching our own Exceptions, any unexpected errors will simply result in the application terminating using the default strategy - and at that point we should pick it up and add better error checking... 😉

About renaming dwipreproc: I can see the arguments for renaming, no problem there. I'm just not sure they're compelling enough to warrant a change that will break users' scripts. And while Gibb's ringing removal, denoising, and bias field correction are all highly recommended, they're relatively recent additions, and I think most people would agree that the single most important preprocessing steps in DWI preprocessing in general are those currently included in dwipreproc. So I'd rather leave it as-is for now...

We tend not to use it and rely on catching our own exceptions ...

Huh, OK. I tend to use ellipsis, with the exception (ba-dum-tish) being those cases where I want to concatenate the exception message (which I'm trying to do increasingly: many forum questions have boiled down to exceptions thrown deep in the stack that users can't translate to their command invocation). That's the pattern I recognized having looked through others' code, anyway. If you'd rather have non-Exception exceptions not caught, we should do that throughout; just do catch (Exception& /*unused*/) so there aren't unused variable warnings.

So I'd rather leave it as-is for now...

👍 No worries, that's fair enough. It would be good to keep an eye on it though, as time goes on.

mrmath seems to segfault during some mrfilter tests though, as well as during some tensor2metric tests.

Fixed... At some point I saw this line and went "oh, you can erase map entries without checking for their existence!". Yeah, nah, you can't... 🙄