[ENH] Update ReconAll interface for v6

[effigies]

This is more of an RFC with some attached code than a PR, at the moment.

Below are (most of) the changes to ReconAll._steps needed to reflect the FreeSurfer v6.0 recon-all table, rather than the v5.3 recon-all table.

There are some additions, deletions as well as different outputs. So a few questions to begin the discussion:

1. Is the _steps approach still good? Note that aseg.mgz is the output of two different steps, which means the latter almost certainly won't be run without actually checking whether outputs are newer than inputs.
2. Do we want to include -T2pial/-FLAIRpial or leave it as something people can specify in ReconAll.inputs.args?
3. Given that there exists a Makefile-based recon-all maintained by the FreeSurfer devs, does it make sense to do our own dependency management?

---

Fixes #1782.

[satra]

@effigies - thanks for bringing up these questions

1. Is the _steps approach still good? Note that aseg.mgz is the output of two different steps, which means the latter almost certainly won't be run without actually checking whether outputs are newer than inputs.

the steps approach was there to restart at the appropriate point in the recon-all workflow and there was something off about simply using the make option, but if the make option is cleaner and it works i would support removing steps.

2. Do we want to include -T2pial/-FLAIRpial or leave it as something people can specify in ReconAll.inputs.args?

https://github.com/nipy/nipype/blob/master/nipype/interfaces/freesurfer/preprocess.py#L621

3. Given that there exists a Makefile-based recon-all maintained by the FreeSurfer devs, does it make sense to do our own dependency management?

not if it works.

on a related note, there will be some work towards moving the recon-all script to a more maintainable mode by the freesurfer folks, and we are definitely hoping to converge towards sth that we can use to generate a parallelizable nipype workflow. the intent behind the work from iowa was to figure out ways to improve or replace pieces of recon-all (for example, replacing n3biascorrection with n4 or taking out mni tools, etc.,.).

[codecov-io]

Codecov Report

Merging #1790 into master will not impact coverage by -0.01%.

@@            Coverage Diff             @@
##           master    #1790      +/-   ##
==========================================
- Coverage   69.82%   69.82%   -0.01%     
==========================================
  Files        1047     1047              
  Lines       52050    52089      +39     
  Branches     7651     7657       +6     
==========================================
+ Hits        36345    36369      +24     
- Misses      14127    14133       +6     
- Partials     1578     1587       +9

Flag	Coverage Δ
#unittests	69.82% <70.37%> (-0.01%)	❌

Impacted Files	Coverage Δ
nipype/utils/filemanip.py	82.78% <100%> (+0.25%)	✅
nipype/interfaces/freesurfer/preprocess.py	64.33% <37.5%> (-0.05%)	❌
nipype/utils/tests/test_filemanip.py	93.21% <96.15%> (+0.35%)	✅
nipype/pipeline/plugins/tests/test_multiproc.py	72.32% <ø> (-6.29%)	❌
nipype/interfaces/dynamic_slicer.py	17.47% <ø> (ø)	✅
nipype/interfaces/nipy/preprocess.py	47.56% <ø> (ø)	✅
nipype/interfaces/io.py	57.55% <ø> (ø)	✅

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d4d5e4...06a9eb9. Read the comment docs.

[effigies]

Ah, looked right at the -T2pial and didn't see it.

So, looking at recon-all -make, setting MAKEFLAGS="-j -l <NCORES>" causes it to try to do things out of order (starts trying to smooth non-existent surfaces), presumably due to an incomplete dependency graph. That's not great. Still, it does pass through -parallel -openmp <NCORES>, so we wouldn't be losing anything by going through make. I'll see if I can make that work and resume properly in NiPype.

The simplest alternative I see is to add an optional dependencies field to steps, checking mtimes if present. That way, at least, if people need to re-run after manual edits, there'll be an existing data structure to update as needed.

One other thing about steps, though, is that, if you ever complete a step in autorecon3, the directive will always be -autorecon3, regardless of whether you've touched up a file and need to rerun parts of autorecon2 (see https://surfer.nmr.mgh.harvard.edu/fswiki/recon-all#SpecifyingDirectives for examples of times you might want that). Fixing this may be a bit out of scope for just getting this so you can run ReconAll from start-to-finish, and probably a poor use of time if there's work toward re-architecting. Would be glad to participate in existing efforts, there.

[effigies]

Nope. -make doesn't handle -no* arguments. Updating steps, and added a third field for optionally specifying dependencies. check_deps tests target existence and whether targets are newer than dependencies. If this is an acceptable addition, I'll write some tests.

I've also removed the directive override. Reasoning from commit:

Assuming any directive maps to '-all' with all previous steps completed
is incorrect. Further, the directive update to 'autorecon2' precludes
the completion of 'autorecon3', which 'all' includes.

Taking the existing directive and removing completed steps is a more
conservative approach, with less chance of unexpected behavior.

If you want to put something else to replace it, I can, but I think we risk getting too complicated, here. Following the user's directive, and just removing the completed bits (extraneous -no* flags won't modify behavior) seems like the desired behavior.

[satra]

@effigies - just to be sure this interface will work with 5.3 and 6 correct?

[effigies]

Ah, no. At present, it just replaces the 5.3 _steps. I'll re-add, and make a version check.

[effigies]

Travis passing on my account. The error there is one of those random segfaults.

@satra Ready for review.

[chrisgorgo]

Don't we also need to add the -parallel flag?

[effigies]

Good call. For some reason I got it in my head that -openmp implied -parallel. Any other flags worth giving inputspec entries? Such as -hires, -use-cuda/-use-gpu, or -clean?

[chrisgorgo]

We'll need -hires eventually, but this can wait for another PR.