Skip brain extraction #116

adelavega · 2019-10-07T23:36:18Z

Fixes #12 by creating an alternative workflow to init_brain_extraction_wf, init_n4_only_wf.
This wf only does n4 bias field correction, and creates a mask file by thresholding the input T1w image.

My main concern is if this is the right approach.
Alternatively, the init_brain_extraction_wf workflow could be altered in niwokflows, but as this is supposed to be an implementation of ANTS' brain extraction, I thought it was better not to touch that.

I also though about just changing the init_anat_preproc_wf function to change the connections based on if brain extraction would be run, but that got messy quick.

Also, I'm currently just using a parameter to indicate if brain extraction should be skipped. Although the function to detect if brain extraction is needed is easy, I'm not sure how to dynamically create a workflow based on the output of a Node.

Any pointers here?

oesteban

I'll let you keep working on this, but it looks it's taking a good direction.

My main concern is: if the image is skull-stripped, then it is likely to have been INU-corrected too. That makes me think about three issues:

How would you account for INU-corrected images? I mean, it doesn't hurt to run INU-correction twice (in principle) and let N4 find nothing if the image is intensity corrected.
The current antsBrainExtraction workflow is modified to recalculate the B1 field using the Atropos segmentation and passing in the white-matter class. Could we also implement something like that here?
If down the line we decide to implement some B1 field correction in fMRIPrep (and we will certainly do in dMRIPrep), could we break the workflow at construction if some flag (i.e., disallow_premasked) is present set true? This way, dMRIPrep could set it on and prevent this behavior to happen while making noise enough to get the attention of the user.

smriprep/workflows/anatomical.py

adelavega · 2019-10-08T02:53:24Z

Thanks for taking a look.

Yes that's what I was thnking and what @effigies had suggested. It shouldn't in principle hurt to run that twice, whereas running brain extraction twice does hurt
Sure, I just wasn't sure if that was necessary, but that'd be easy enough. In that case though, would you rather keep this workflow here, or modify the ants one to just skip brain extraction given a flag? Again, I can understand how keeping that one strictly like the ants workflow makes sense so I have no strong opinion
Makes sense

Any suggestions re: the brain-mask detection though? Or are you happy to leave it as an explicit CLI option?

adelavega · 2019-10-11T16:54:30Z

I successfully ran ds001110/SherlockMerlin with brain extraction turned off, and the results look decent as far as I can tell.

This is ready for another review whenever you get a chance. No rush from me as preprocessing that dataset was my main goal.

Here are the reports for the dataset if anyone cares to take a look: https://web.corral.tacc.utexas.edu/neuroscout-datasets/SherlockMerlin/fmriprep/

Main points left to discuss:

Should this behavior be handled through a flag or detection? Either way I think that should go into fmriprep and this is fine as is. If we do detection, I guess we need to do it outside of nipype using pybids + the function I posted before. (Unless anyone knows how to do a conditional graph)

Should this new workflow go here or in niworkflows? Up to you. I'm happy to open another PR there with just the init workflow function.
The disallow_premasked flag only makes sense in context of detection, right? If so, I'll wait until we resolve 1)

cc: @effigies

adelavega · 2019-10-11T16:55:52Z

One more thing: I noticed CopyXForm sneakily also does a "pop" off a list. Thus, I put it back in, partly because its used for those outputs in the other workflow, and thus might be necessary.

effigies · 2019-10-11T18:04:46Z

Cool. Will have a look now. Should be able to fix the tests by merging/rebasing master.

pep8speaks · 2019-10-11T19:16:54Z

Hello @adelavega! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-07 20:47:11 UTC

effigies · 2019-10-11T19:18:47Z

Haven't configured the PEP8 speaks... Ignore the line lengths.

effigies · 2019-10-11T19:19:17Z

Also, something's come up, and I might not finish a review this afternoon.

adelavega · 2019-10-14T21:42:19Z

update: I didnt realize brain_extraction_wf gets the average T1w as input. In that case, the thresh_low of 0 wasn't high enough and resulted in masks like:

I think this led to a series of errors which prevented confounds from being extracted.

I set it to 10, and that looks pretty close to what you get by threshold a single T1w with 0. A treshold of 1 should also be acceptable. I have no clue what the range of values of T1w images tend to be.

I'll re-run with these values.

adelavega · 2019-10-14T23:17:46Z

I undid the merge master (at least temporarily), because it seems that fmriprep is lagging behind smriprep (specifically in the naming of the outputs that was recently done)

Co-Authored-By: Oscar Esteban <[email protected]>

effigies · 2019-12-20T12:11:58Z

What if we ask users to provide a brain mask? Then we just use it.

adelavega · 2019-12-20T17:29:10Z

But then it would have to be per subject, and I think the brain mask extraction works pretty well (so why create more work for the user?)

Given this is fairly niche (although IMO important, given the # of openneuro datasets like this), I think a CLI flag might be a good and easy solution (especially since we would need an over-ride CLI flag anyways, so were adding yet another option no matter what).

…into enh/skull_strip_skip

adelavega · 2020-01-14T18:05:56Z

Before we all forget about this PR, let me summarize:

The n4_only workflow is merged and released on niworkflows
The skull-strip detection seems to work fairly well, but we are concerned about edge cases

So we need to decide if s/fmriprep should either:

Detect skull stripped T1ws, with a flag to disable detection and --force-skull-strip. This is good if we think there will be false positives (non-stripped T1ws detected as pre-stripped).
The same as above, but instead of force, a --skip-skull-strip flag. This is good if we think there will be false negatives.
A flag with the following options: auto (default) force & skip. This is the most flexible.
No detection, only a --skip-skull-strip flag

Given that the detection could fail either way (although honestly, I think it's probably unlikely), I would vote for 3).

I'm also fine with 4), as I think users with pre-stripped datasets may already know it (although I didn't at first, and I was puzzled as to why my results looked bad).

effigies

Alas, I forgot about this PR. But thanks for the summary.

I'd be okay with a --skip-skull-strip flag (or maybe --pre-skull-stripped?) to override detection.

Some additional comments, but otherwise this is looking good. We're going to have merge conflicts with #159. IDK if you want to preemptively merge/rebase on that, or try to squeeze this in and make @mgxd do the work.

smriprep/workflows/anatomical.py

adelavega · 2020-01-29T20:22:33Z

DK if you want to preemptively merge/rebase on that, or try to squeeze this in and make @mgxd do the work.

Ahem, the latter if possible.

Co-Authored-By: Chris Markiewicz <[email protected]>

effigies · 2020-01-29T21:42:45Z

smriprep/workflows/base.py

@@ -329,7 +329,7 @@ def init_single_subject_wf(
        hires=hires,
        longitudinal=longitudinal,
        name="anat_preproc_wf",
-        num_t1w=len(subject_data['t1w']),
+        t1w=subject_data['t1w'],


You need to pass skip_brain_extraction here, and up the chain to the CLI.

effigies · 2020-01-29T21:43:13Z

Need to update the niworkflows dependency here:

https://github.com/poldracklab/smriprep/blob/6a556bcf16a33ea21ed4fdcb8b7335332af10654/docs/requirements.txt#L5

effigies · 2020-01-29T21:45:31Z

Pulling from a resolved comment:

We also need to update workflow.__desc__ to accurately reflect:

Skull-stripping is run

Skull-stripping is skipped

Pre-skull-stripped images were detected, and so skull-stripping was skipped.

effigies · 2020-01-29T22:46:52Z

smriprep/cli/run.py

@@ -95,6 +95,9 @@ def get_parser():
    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(
+        '--skip-brain-extraction', required=False, action='store_true',
+        default=False, help='Skip the brain extraction workflow (for pre-extracted T1ws))')


Hmm. This takes care of false negatives, but I suppose we should consider false positives, where we might incorrectly decide an image is skull-stripped.

Maybe we want --skull-strip {auto,skip,force}?

Or am I over-thinking the problem?

Yeah, I previously suggested this, but also worried I was overthinking it. False positives are actually probably a bigger problem in a way (many more people have normal T1s)

If you're comfortable with the additional CLI complexity, I think it this makes the most sense.

Again, we could also remove the detection if we think its not reliable, and only have a skip detection flag.

My preferences in order: 1) CLI argument w/ 3 options 2) skip only CLI flag, with no detection 3) skip flag + detection (current)

I lean toward 1 as well.

@mgxd ? any opinion

i'm fine with the 3 way approach, with the default being auto. It should definitely document which route was taken

Sure, how? You mean which option was select or what auto detected? And if so, where do I log that?

mgxd

cool! couple comments

mgxd · 2020-01-30T18:28:49Z

smriprep/workflows/anatomical.py

@@ -22,15 +25,17 @@
 )
 from niworkflows.interfaces.images import TemplateDimensions, Conform, ValidateImage
 from niworkflows.utils.misc import fix_multi_T1w_source_name, add_suffix
-from niworkflows.anat.ants import init_brain_extraction_wf
+from niworkflows.anat.ants import (
+    init_brain_extraction_wf, init_n4_only_wf, _pop


not a huge fan of importing "private" functions

Sure, it's just a pop function so we can redefine.

mgxd · 2020-01-30T18:29:41Z

docs/requirements.txt

why is this different? spacing?

not sure 🤷

mgxd · 2020-01-30T18:46:27Z

smriprep/cli/run.py

@@ -95,6 +95,9 @@ def get_parser():
    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(
+        '--skip-brain-extraction', required=False, action='store_true',
+        default=False, help='Skip the brain extraction workflow (for pre-extracted T1ws))')


i'm fine with the 3 way approach, with the default being auto. It should definitely document which route was taken

effigies · 2020-02-05T18:04:49Z

Looks like @mgxd got in first. Merge/rebase?

mgxd

I'm gonna cut a new release in order to get an fmriprep release out, but this is close. I can clean up the git history once all the above is addressed 👍

smriprep/workflows/anatomical.py

Co-Authored-By: Mathias Goncalves <[email protected]>

…into enh/skull_strip_skip

effigies · 2020-02-12T18:06:12Z

Closed in favor of #167.

adelavega added 2 commits October 7, 2019 18:25

Build alternative workflow skipping brain extraction

b2de82b

Skip brain extraction false by default

6b65452

oesteban reviewed Oct 8, 2019

View reviewed changes

smriprep/workflows/anatomical.py Outdated Show resolved Hide resolved

smriprep/workflows/anatomical.py Outdated Show resolved Hide resolved

smriprep/workflows/anatomical.py Outdated Show resolved Hide resolved

smriprep/workflows/anatomical.py Outdated Show resolved Hide resolved

adelavega added 7 commits October 8, 2019 13:11

Use nibabel Binarize to create brain mask

e4ef18e

Dont import Registration

f7381d4

Add atropos n4 refinement to n4only workflow

bf331f6

Lint, and pass args to new workflow

365267a

Remove copy_xform

bf97ff9

Bump niworkflows to 0.10.4, and fix usage of Binarize

8a3fcb1

in_file not input_image in Binarize

0b90071

adelavega force-pushed the enh/skull_strip_skip branch from cef24eb to 0b90071 Compare October 9, 2019 18:33

adelavega added 7 commits October 9, 2019 17:04

pop first mask

395ecb6

In_file not input_image

34ab284

Add back CopyXForm, for n4 outputs

6f419c8

Resolved conflict

57b47f4

Fix connection

580f090

Binarize in/outs

cecd376

hdr_file

2e36d81

Set thresh_low to 10

7ee3c6a

adelavega force-pushed the enh/skull_strip_skip branch from 83ba99c to 7ee3c6a Compare October 14, 2019 23:16

Out mask, not out file

179686e

Update smriprep/workflows/anatomical.py

2938b7f

Co-Authored-By: Oscar Esteban <[email protected]>

adelavega added 3 commits December 20, 2019 11:46

Check for near zero values, and correct logic in _is_skull_stripped"

c314274

Merge branch 'enh/skull_strip_skip' of github.com:adelavega/smriprep …

31bcc81

…into enh/skull_strip_skip

Add imports for numpy, nibabel

583f9b3

emdupre mentioned this pull request Jan 14, 2020

[ENH] Derived (processed) MR data bids-standard/bids-specification#207

Closed

effigies reviewed Jan 29, 2020

View reviewed changes

smriprep/workflows/anatomical.py Outdated Show resolved Hide resolved

smriprep/workflows/anatomical.py Outdated Show resolved Hide resolved

smriprep/workflows/anatomical.py Outdated Show resolved Hide resolved

adelavega and others added 2 commits January 29, 2020 14:23

Apply suggestions from code review

71180eb

Co-Authored-By: Chris Markiewicz <[email protected]>

Make skip_brain_extraction a positional argument

836d5ac

effigies reviewed Jan 29, 2020

View reviewed changes

Propagate skip_brain_extraction to CLI

a13fdf5

effigies reviewed Jan 29, 2020

View reviewed changes

PIN: Bump niworkflows to 1.1.4

cde34af

mgxd reviewed Jan 30, 2020

View reviewed changes

mgxd reviewed Feb 6, 2020

View reviewed changes

smriprep/workflows/anatomical.py Outdated Show resolved Hide resolved

mgxd modified the milestones: 0.5.0, 0.6.0 Feb 6, 2020

adelavega and others added 4 commits February 6, 2020 16:37

Update smriprep/workflows/anatomical.py

e98e2ed

Co-Authored-By: Mathias Goncalves <[email protected]>

Don't import private _pop

a8845fb

Add three option CLI

eceef1e

Merge branch 'enh/skull_strip_skip' of github.com:adelavega/smriprep …

183db21

…into enh/skull_strip_skip

mgxd mentioned this pull request Feb 11, 2020

ENH: Add option to skip brain extraction #167

Merged

effigies closed this Feb 12, 2020

Skip brain extraction #116

Skip brain extraction #116

Uh oh!

Conversation

adelavega commented Oct 7, 2019

Uh oh!

oesteban left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adelavega commented Oct 8, 2019

Uh oh!

adelavega commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adelavega commented Oct 11, 2019

Uh oh!

effigies commented Oct 11, 2019

Uh oh!

pep8speaks commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-02-07 20:47:11 UTC

Uh oh!

effigies commented Oct 11, 2019

Uh oh!

effigies commented Oct 11, 2019

Uh oh!

adelavega commented Oct 14, 2019

Uh oh!

adelavega commented Oct 14, 2019

Uh oh!

effigies commented Dec 20, 2019

Uh oh!

adelavega commented Dec 20, 2019

Uh oh!

adelavega commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adelavega commented Jan 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Jan 29, 2020

Uh oh!

effigies commented Jan 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgxd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adelavega commented Oct 11, 2019 •

edited

Loading

pep8speaks commented Oct 11, 2019 •

edited

Loading

adelavega commented Jan 14, 2020 •

edited

Loading