Skip to content

ENH: Add -hires and -expert flags to ReconAll #1857

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 4, 2017

Conversation

effigies
Copy link
Member

@effigies effigies commented Mar 1, 2017

FreeSurfer 6 supports a -hires flag for submillimeter reconstruction. Currently, it's recommended that an expert options file be provided to modify default mris_inflate parameters, so I've also added the -expert flag, though it is not new.

@effigies
Copy link
Member Author

effigies commented Mar 1, 2017

At present, this assumes people will create their own expert files. I could add a new interface to be used like:

expert_file = pe.Node(ExpertFile(mris_inflate='-n 15'), name='ExpertFile')

That produces the file

mris_inflate -n 15

As a sketch of the specs limiting the options to programs that will accept expert files (there might be a better way):

class ExpertFileInputSpec:
    talairach = traits.Str(desc="Flags to pass to talairach commands")
    ...
    mri_aparc2aseg = traits.Str(desc="Flags to pass to mri_aparc2aseg commands")

class ExpertFileOutputSpec:
    out_file = File(exists=True, desc="Expert options file")

@effigies
Copy link
Member Author

effigies commented Mar 1, 2017

Just to note: Segfault still occurring sometimes on Travis.

@codecov-io
Copy link

codecov-io commented Mar 1, 2017

Codecov Report

Merging #1857 into master will increase coverage by 0.01%.
The diff coverage is 86.36%.

@@            Coverage Diff             @@
##           master    #1857      +/-   ##
==========================================
+ Coverage   73.04%   73.05%   +0.01%     
==========================================
  Files        1064     1064              
  Lines       53367    53411      +44     
==========================================
+ Hits        38981    39019      +38     
- Misses      14386    14392       +6
Flag Coverage Δ
#unittests 73.05% <86.36%> (+0.01%)
Impacted Files Coverage Δ
.../interfaces/freesurfer/tests/test_auto_ReconAll.py 100% <ø> (ø)
nipype/interfaces/freesurfer/preprocess.py 69% <86.36%> (+0.85%)

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 0366859...2aa325e. Read the comment docs.

@effigies
Copy link
Member Author

effigies commented Mar 2, 2017

Added the ExpertOptions interface since I wanted to experiment with using it. Probably won't need it after all (will just manually create the file), so I'll only keep it if it seems like a good idea in general.

@chrisgorgo
Copy link
Member

Isn't the ExpertOptions interface a little bit of an overkill? I mean it only produces one config file and it will only be connected to ReconAll node (correct me if I am wrong). Wouldn't it be more straightforward to fold this into ReconAll itself?

We have created on demand config files on other occasions https://github.com/nipy/nipype/blob/master/nipype/interfaces/dcm2nii.py#L195

@effigies effigies force-pushed the reconall_newparams branch from b824128 to 1941987 Compare March 3, 2017 19:05
@effigies
Copy link
Member Author

effigies commented Mar 3, 2017

@chrisfilo How do you feel about this fold-in? It accepts either an existing file or a dictionary of {binary: options_string} and creates a file if needed. (This doesn't check for valid binaries, unlike the spec'd interface.)

I'm also okay with simply dropping file creation.

@@ -632,6 +633,10 @@ class ReconAllInputSpec(CommandLineInputSpec):
desc="Number of processors to use in parallel")
parallel = traits.Bool(argstr="-parallel",
desc="Enable parallel execution")
hires = traits.Bool(argstr="-hires", min_ver='6.0.0',
desc="Conform to minimum voxel size (for voxels < 1mm)")
expert = traits.Either(File, DictStrStr, argstr='-expert %s',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File(exists=True)?

@chrisgorgo
Copy link
Member

Hmm... now we are not exposing all of the possible dict keys. What about adding all of those inputs to ReconAll 1941987#diff-08eb19cbbabcb3f9cef1bb3197b661ecL2885 and xoring on the expert_file input? This would have an advantage of people knowing straight away what are the possible options WDYT?

@@ -933,6 +980,24 @@ def cmdline(self):
iflogger.info('resume recon-all : %s' % cmd)
return cmd

def _prep_expert_file(self):
if isdefined(self.inputs.extra):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I get for pushing before testing. Updated.

@effigies
Copy link
Member Author

effigies commented Mar 3, 2017

@satra How do you feel about this interface? @chrisfilo is right that the expert file is recon-all-specific, but this definitely adds a lot to the input spec.

@effigies effigies force-pushed the reconall_newparams branch from 379e59f to 2aa325e Compare March 3, 2017 21:54
@satra satra merged commit 07a75a6 into nipy:master Mar 4, 2017
@effigies effigies deleted the reconall_newparams branch April 10, 2017 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants