Skip to content

ENH: ReconAll handle less common cases #1966

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 3 commits into from
Apr 27, 2017
Merged

Conversation

effigies
Copy link
Member

This PR is two-in-one. Can be split if needed.

Following on #1790/#1824, the first updates the "steps" checking to only check the steps implied by the directive.

  • Undefined removed the directive flag, and so also the steps checking
  • autorecon2-* are considered the same as autorecon2 for these purposes. I can look more into if they should be done differently if that's desired.
  • Non-autorecon* directives are given the same treatment as all, mainly because it doesn't change the behavior from before, and I haven't looked up what they actually do.

The primary goal is to enable ReconAll(directive=Undefined) to behave as expected when a subject's recon directory already exists. I don't believe that the others will have much effect but avoid throwing in a ton of -no* flags that aren't needed.

Related: nipreps/fmriprep#429


The second commit is an update to expert files, following on #1857. FreeSurfer saves its expert options files on the first introduction. If they are ever set again, they need to be passed with -xopts-overwrite, or else it throws an error. There are also -xopts-use (default) and -xopts-clean. The proposed change does the following:

  • Checks the expert options file to be written against an existing file; if the contents are the same, -xopts-use is passed instead of writing a new expert options file.
  • If an -xopts-* flag is passed by the user, this check is skipped, on the assumption the user knows what they're doing.

An FMRIPREP user encountered an issue with FreeSurfer complaining about needing -xopts-overwrite when due to memory reasons the node which set the expert options crashed and had to be rerun.

Related: nipreps/fmriprep#425

Set default behavior to refrain from creating/passing a new expert
options file if an existing one has the same contents.

Also, give access to  -xopts-{use,clean,overwrite} options, if users
want to override this default.
@codecov-io
Copy link

codecov-io commented Apr 23, 2017

Codecov Report

Merging #1966 into master will decrease coverage by 0.02%.
The diff coverage is 20.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1966      +/-   ##
==========================================
- Coverage   72.52%   72.49%   -0.03%     
==========================================
  Files        1070     1070              
  Lines       54390    54421      +31     
  Branches     7858     7867       +9     
==========================================
+ Hits        39448    39455       +7     
- Misses      13715    13737      +22     
- Partials     1227     1229       +2
Flag Coverage Δ
#smoketests 72.49% <20.58%> (-0.03%) ⬇️
#unittests 70.05% <20.58%> (-0.03%) ⬇️
Impacted Files Coverage Δ
.../interfaces/freesurfer/tests/test_auto_ReconAll.py 100% <ø> (ø) ⬆️
nipype/interfaces/freesurfer/preprocess.py 65.28% <20.58%> (-1.42%) ⬇️

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 dc68922...1db7b23. Read the comment docs.

effigies added a commit to effigies/fmriprep that referenced this pull request Apr 24, 2017
@satra
Copy link
Member

satra commented Apr 24, 2017

@effigies - while we are at this should we add options like -mprage, -bigventricles?

@effigies
Copy link
Member Author

Yeah, I'm fine with that. There are so many options that I don't know what all is worth putting in. Do you have others you use regularly?

@satra
Copy link
Member

satra commented Apr 24, 2017

most of the time i just run with -all but a few have come up that are now relevant to some of the things we are doing - normally i just stick them in the generic args

a few more from the recent release:

 Segmentation of substructures of hippocampus and brainstem
  -hippocampal-subfields-T1 : segmentation of hippocampal subfields using input T1 scan
  -hippocampal-subfields-T2 file ID : segmentation using an additional scan (given by file);
                                      ID is a user-defined identifier for the analysis
  -hippocampal-subfields-T1T2 file ID : segmentation using additional scan (given by file) and input T1
                                        simultaneously; ID is a user-defined identifier for the analysis
  -brainstem-structures : segmentation of brainstem structures

these require the matlab common runtime and very specifically following these instructions:

https://surfer.nmr.mgh.harvard.edu/fswiki/MatlabRuntime

i'm hoping to build a container with everything and then reduce using reprozip for recon-all.

  • run single recon with all options (T1 + T2 + brainstem + hippocampal subfields)
  • run longitudinal version
  • run custom template building

@effigies
Copy link
Member Author

-brainstem is easy enough. Do you know how to encode the hippocampal subfields options in a single trait?

Something like:

hippocampal_subfields = traits.Enum(
    'T1',
    traits.Tuple(traits.Enum('T2', 'T1T2'), File(exists=True), traits.Str),
    argstr='-hippocampal-subfields-%s ...')

But not actually that, because trying ReconAll(hippocampal_subfields=('T2', '/dev/null', 'test')) gives:

TraitError: The 'hippocampal_subfields' trait of a ReconAllInputSpec instance
must be u'T1' or <traits.trait_types.Tuple object at 0x7f6aed881510>, but a
value of ('T2', '/dev/null', 'test') <type 'tuple'> was specified.

Similarly

hippocampal_subfields = traits.Tuple(
    traits.Enum('T1', 'T2', 'T1T2'), File(exists=True), traits.Str(),
    argstr='-hippocampal-subfields-%s ...')

yields:

TraitError: The 'hippocampal_subfields' trait of a ReconAllInputSpec instance
must be a tuple of the form: ('T1' or 'T2' or 'T1T2', an existing file name, a
unicode string), but a value of ('T2', '/dev/null', 'test') <class 'tuple'> was specified.

It seems this should be doable, but I can just do an xor set, too.

File(exists=True), traits.Str(),
argstr='-hippocampal-subfields-T2 %s %s', min_ver='6.0.0',
desc=('segment hippocampal subfields using T2 scan, identified by '
'ID (may be combined with hippocampal_subfields_T1)'))
Copy link
Member Author

Choose a reason for hiding this comment

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

@satra What do you think of this implementation? The tuple might be a little awkward; if you'd prefer, I could make a hippocampal_subfields_id that gets pulled in in _format_arg. If so, is there a reasonable default ID name?

Copy link
Member

Choose a reason for hiding this comment

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

is the file here the same as the T2 input, in which case, should we simply say this field requires T2?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. A separate filename is parsed. I don't know enough about these processes to know if you'd use the same T2 for hippocampal subfields as for pial surface refinement, but certainly it's not required by their code that they be the same file.

@effigies effigies requested a review from satra April 27, 2017 11:59
Copy link
Member

@satra satra left a comment

Choose a reason for hiding this comment

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

LGTM

@satra satra merged commit f3ba8d2 into nipy:master Apr 27, 2017
@effigies effigies deleted the enh/recon_undef branch April 27, 2017 19: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.

3 participants