-
Notifications
You must be signed in to change notification settings - Fork 536
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
Conversation
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 Report
@@ 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
Continue to review full report at Codecov.
|
@effigies - while we are at this should we add options like -mprage, -bigventricles?
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?
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
-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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.
Undefinedremoved the directive flag, and so also the steps checkingautorecon2-*are considered the same asautorecon2for these purposes. I can look more into if they should be done differently if that's desired.autorecon*directives are given the same treatment asall, 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:-xopts-useis passed instead of writing a new expert options file.-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-overwritewhen due to memory reasons the node which set the expert options crashed and had to be rerun.Related: nipreps/fmriprep#425