Skip to content
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

ENH: Add a --skull-strip-t1w argument to skip brain extraction #2039

Merged
merged 4 commits into from
Mar 19, 2020

Conversation

oesteban
Copy link
Member

@oesteban oesteban commented Mar 17, 2020

Supersedes #1829.

@adelavega can you check locally this works for you?

@oesteban oesteban requested review from adelavega and mgxd March 17, 2020 18:52
@auto-comment

This comment has been minimized.

Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

lgtm

docs/workflows.rst Outdated Show resolved Hide resolved
@effigies
Copy link
Member

Why is the default not "auto"?

@adelavega
Copy link
Contributor

adelavega commented Mar 17, 2020

I'd think auto should be the default as well, otherwise if a user is changing that option its likely they know they need to skip brain extraction. That said, I can understand if we don't think the algorithm is reliable enough (yet?)

Will try this out soon

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Some alternative wordings, if we do switch to auto as default (which I think we should).

@@ -439,6 +439,9 @@ class workflow(_Config):
"""Fix a seed for skull-stripping."""
skull_strip_template = "OASIS30ANTs"
"""Change default brain extraction template."""
skull_strip_t1w = "force"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
skull_strip_t1w = "force"
skull_strip_t1w = "auto"

@@ -209,6 +209,11 @@ def _bids_filter(value):
g_ants.add_argument('--skull-strip-fixed-seed', action='store_true',
help='do not use a random seed for skull-stripping - will ensure '
'run-to-run replicability when used with --omp-nthreads 1')
g_ants.add_argument(
'--skull-strip-t1w', action='store', choices=('auto', 'skip', 'force'), default='force',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'--skull-strip-t1w', action='store', choices=('auto', 'skip', 'force'), default='force',
'--skull-strip-t1w', action='store', choices=('auto', 'skip', 'force'), default='auto',

docs/workflows.rst Outdated Show resolved Hide resolved
Co-Authored-By: Chris Markiewicz <effigies@gmail.com>
@oesteban
Copy link
Member Author

I'd rather keep the 'force' mode as default, since it is currently the "official" operation mode. This flag was suggested to cover a particular use case (when the user is not aware of having T1w images already masked), and crashing first seems like a conservative but effective approach.

Finally, this feature should be superseded by evolutions of nipreps/smriprep#107 (and #2036) where one can feed fMRIPrep with subsets of precalculated anatomical derivatives.

Is that convincing?

@effigies
Copy link
Member

I don't see the point of having auto at all, then. The idea was to have something that didn't require using a CLI flag most of the time.

@oesteban
Copy link
Member Author

Removing 'auto' would effectively be the same as #1829 (i.e., a boolean switch). However, when writing this I thought it was worth having 'auto' for when your dataset has both full and brain-extracted T1w, and you don't know which participants while you trust the heuristic. I can add a warning saying "use with caution" to emphasize this point.

Without a benchmark that says the heuristic works with very high sensitivity and specificity, I feel resistant to making it the default.

@oesteban
Copy link
Member Author

oesteban commented Mar 18, 2020

For instance @nicholst et al., are processing ds001 and have found fMRIPrep gives them very bad results for sub-04.

Looking at the reports, they discovered that the T1w is skull-stripped.

I think having an implicit option would have been potentially dangerous to their type of analysis because it would have been really easy to miss that fMRIPrep actually ran different workflows for that subject w.r.t. the others (and this is assuming 'auto' worked perfectly on all participants).

Setting the default to 'force' (i.e., the original fMRIPrep behavior before this patch) is more explicit.

@oesteban oesteban added this to the 20.1.0 milestone Mar 18, 2020
@nicholst
Copy link

Setting force as default makes sense for continuity of usage. If people get some experience with auto and find it does a good job catching extracted brains, then later it could be switched to auto.

@adelavega
Copy link
Contributor

adelavega commented Mar 18, 2020

Works for me. FWIW I just tested in on ds001110 with auto and it worked well (detected skull stripped T1 and skipped the workflow).

@oesteban
Copy link
Member Author

Thanks for the fast check and your patience, @adelavega. This took a lot longer than we would've liked (for the most part, because of me) - many thanks.

@mgxd mgxd added this to Blocking in 20.1 Mar 19, 2020
@oesteban oesteban mentioned this pull request Mar 19, 2020
@oesteban oesteban merged commit 0482c23 into nipreps:master Mar 19, 2020
@oesteban oesteban deleted the enh/skip-anat-brain-extraction branch March 19, 2020 16:55
@mgxd mgxd moved this from Blocking to Complete in 20.1 Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
20.1
Complete
Development

Successfully merging this pull request may close these issues.

None yet

5 participants