Agree about making FFTW a hard dependency for MRtrix3

From prior discussions I believe that this is the way to go.

Could however have configure set an envvar that would precompile out all of mrdegibbs and the fft filter in mrfilter if the FFTW library is not present?

• Decide whether we want a dedicated option to activate 3D processing (e.g. -3d), rather than the current -axes 0:2

I think this is probably better. -axes 0:2 versus -axes 0,2 seems prone to errors.

• Clean up existing FFT filter code to remove all reliance on Eigen/Unsupported FFT code, and use FFTW directly
• Agree about making FFTW a hard dependency for MRtrix3

Is there a problem with the Eigen/Unsupported FFT code? Is there a big difference in run time? I would prefer not to need the extra dependency...

• Decide whether we want a dedicated option to activate 3D processing (e.g. -3d), rather than the current -axes 0:2

I think this is probably better. -axes 0:2 versus -axes 0,2 seems prone to errors.

Yes, I had a feeling that would be the consensus.

Is there a problem with the Eigen/Unsupported FFT code? Is there a big difference in run time? I would prefer not to need the extra dependency...

Yes, it's considerably slower than FFTW. I really wouldn't recommend using the Eigen FFT code if the FFTW library was available. And it's very widely available on all target platforms, which is why I'm actually pretty comfortable with making it a hard requirement. More to the point: there's a few idiosyncracies between the FFTW Eigen backend and their native backend, which make it a bit fragile to support both - especially when used in multi-threaded apps. I'd rather avoid the headaches....

Could however have configure set an envvar that would precompile out all of mrdegibbs and the fft filter in mrfilter if the FFTW library is not present?

If there was any suggestion that installing FFTW might be difficult on some platforms, I might consider this. But given how easy it is on all the platforms we target, I'd prefer to keep things simple both for us and our users by making FFTW a hard requirement, and making sure users always have dwidenoise and the FFT filter in mrfilter if they have MRtrix installed. Otherwise, things are going to get needlessly fragmented...

OK, happy with that.

I think this is probably better. -axes 0:2 versus -axes 0,2 seems prone to errors.

Good point, though I don't know that the former should be forbidden; -3d would essentially be an alias alternative for -axes 0:2 / -axes 0,1,2.

given how easy it is on all the platforms we target, I'd prefer to keep things simple both for us and our users by making FFTW a hard requirement

Haven't looked into ubiquity of the library, so if that's your assessment then 👍

OK, I think this last commit addresses all the mrdegibbs-related comments. Only issue is that it's not possible to use the -3d option as the cmdline parsing backend interprets this as a number that can't be interpreted as an option. I've used the -volume option instead to enable volume-wise mode - open to other suggestions...

Still to do: tidy up the FFT filter.

Is it intended that -3d is interpreted as a number despite the letter in it? If not, personally, I'd suggest to fix the command line parsing if not too much effort and favour -3d over -volume. Also, to add my 2p: I think both colon and comma syntax should be valid to make it consistent with the other commands.

Would it support a single axis as well?

Is it intended that -3d is interpreted as a number despite the letter in it?

Command-line entries with a dash then a digit are rejected as command-line option candidates pretty early. We could theoretically ease that restriction by utilising the same trick as in #1794: Take the entry string after the dash(es), attempt to convert to int/float (where crucially the conversion needs to consume the entire string), and if both fail, try interpreting the entry as a command-line option instead.

The hidden catch here is that it would no longer be permissible to have command-line options called -nan or -inf, as it would be impossible to distinguish those from floating-point arguments.

Generally I think not permitting command-line options to start with digits assists in the visual discrimination between options and values. Though if that were the decision, it might be preferable to catch programmers attempting to add a command-line option that starts with a digit and throw an exception immediately upon interface construction, rather than leading to a parsing error upon use.

Would it support a single axis as well?

That would be easy to do technically, but would require yet another code path... Is that something you're interested in doing?

Is it intended that -3d is interpreted as a number despite the letter in it?

I had a think about how that might work, but it's tricky to get right. The main issue as far as I can tell is making sure we don't trip over ourselves due to option shortening. If we're not careful, the number -3 will be interpreted as the option -3d because no other option starts with a 3... Currently we simply assume that if the first non-dash character is a digit or a dot, then it'll be a number and so should be interpreted as an argument rather than an option. The alternative is to try to match to an option if it starts with a dash, matches to the start of one and only one known option, and contains at least one letter. If not, we accept it as an argument only if it can be converted in its entirety to a number without error. I think that would cover all edge cases, but I'd need to let that simmer for a while...

The hidden catch here is that it would no longer be permissible to have command-line options called -nan or -inf, as it would be impossible to distinguish those from floating-point arguments.

Well, the flip-side is that it's not currently possible to use -nan or -inf as an argument (since it'll match this condition), which I think is the bigger limitation (there may be legitimate use cases in e.g. mrcalc). With the above plan, I think we'll be able to accept -nan as an option if it's specified in usage(), and also accept it as an argument otherwise (but I don't think we'll be able to accept them as an argument is they're also specified as options - which I think is fundamentally ambiguous anyway).

I think both colon and comma syntax should be valid to make it consistent with the other commands.

You mean -axes 0:2 and -axes 0,1,2 should both be acceptable and trigger 3D operation? That's already the case... The only case that isn't currently handled is -axes 0,2,1 or other out-of-order lists. I'd need to sort the array first before comparing, and I haven't bothered doing that. I suppose I could do that for completeness...

The main issue as far as I can tell is making sure we don't trip over ourselves due to option shortening.

Good point; I'd advocate for avoiding the prospect for that reason.

OK, this is almost there. Still outstanding is Rob's suggestion:

Consider issuing a WARNING level message if the user has requested 3D operation yet the key-value field SliceDirection is present indicating that the acquisition was in fact 2D.

[EDIT: my mistake, this was already included with 082a832]

Otherwise, you'll note I've changed the option to trigger 3D operation from -volume to -mode 3d, which I reckon makes more sense given that -3d isn't possible. Also provides -mode 2d, which is the default anyway, so I don't expect it to be used, but it does allow for different potential modes of action in the future (e.g. to deal with partial Fourier, as recently proposed...).

OK, this is good to go as far as I can tell. I'll merge now...