Diffusion echo spacing units

[coalsont]

Allows using harmonized time units across pipelines.

[mharms]

Looks good. My only comment would be rather we want to label the --echospacing option as "DEPRECATED" in the usage text (as you've chosen to do currently).

[coalsont]

Do you have an opinion? Are you suggesting it shouldn't be marked as deprecated?

I suppose we should also edit the Batch script to use seconds.

[mharms]

Yeah, I'm inclined to NOT label it as "deprecated".

[demsarjure]

Looks good.

[coalsont]

@mharms So, the reason for implementing the seconds option was that milliseconds was inconsistent with the other pipelines. This implies that we want users going forward to use seconds, to reduce confusion. To me, deprecation just means "there is a better way to do this, please use it instead", with a possibility of removing the deprecated thing at some point (my thinking is whether it would benefit users for it to be removed, which may resolve to "probably never" in this case, and many other cases).

I am aware of other people having different definitions of deprecation ("deprecated things must be removed in X major versions" being one I strongly disagree with and have argued against), so I can understand it may sound scary to people with those kinds of expectations, but I don't have much sense of how much of an issue that is.

What do you see as the cost/benefit of saying "DEPRECATED" versus not?

[mharms]

To me, the addition of the --echospacing-seconds is most useful in just calling additional attention to the units issue for those using the pipeline for the first time. I think the odds that we actually remove the millisecond option are low (why break existing scripts for this?), and in that sense I think calling it "DEPRECATED" is unnecessary, as it may cause others that happen to notice it to wonder if they need to update their scripts for this. Do we have any other options in other pipelines described as DEPRECATED currently?

[glasserm]

I prefer to deprecate as well. Let's have folks be consistent across pipelines in the future.

[mharms]

If we are trying to be consistent, should we go further and set up PreEddy to take both millisec and second-based options as well?

[coalsont]

I don't expect anyone to manually call just PreEddy, I would say converting it is unnecessary work. Similarly, if we wanted to convert it anyway, it would not need both options, either, as I don't think it is user-facing (in our usual conventions, it would be in a scripts subfolder). Personally, I don't see much benefit to having diffusion split into three sub-scripts.

[coalsont]

As for other pipelines using "DEPRECATED", I think this is the first time we are adding a parameter specifically to use different semantics than the previous equivalent parameter, so this situation may not have come up before. Previous changes were mostly to rename options without changing their semantics (the way newopts handles simple renames, the old name isn't even listed in the help info, but still works).

[mharms]

If you and @glasserm want to include "DEPRECATED" in the usage text for --echospacing that's fine with me.

[coalsont]

@demsarjure can you test this branch on diffusion data?

[demsarjure]

Sure, I will fire up some diffusion tests on this branch.

[demsarjure]

@coalsont I ran a couple of tests (some also used the new echospacing-seconds parameter) and there were no issues, so I think you can safely merge this.