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] Automatically use Nilearn's EPI mask when no explicit mask is provided #226

Merged
merged 7 commits into from
Mar 4, 2019

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Feb 27, 2019

References #113.

Changes proposed in this pull request:

  • When an explicit mask is not provided, use nilearn.masking.compute_epi_mask on the first echo to define the initial mask, which will be adjusted by make_adaptive_mask. The first echo should have the least dropout, and thus should produce the largest mask, but will avoid problems we've been seeing where the adaptive mask includes almost all of the voxels in the bounding box.

@codecov
Copy link

codecov bot commented Feb 27, 2019

Codecov Report

Merging #226 into master will decrease coverage by 1.07%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
- Coverage   51.39%   50.31%   -1.08%     
==========================================
  Files          32       32              
  Lines        1975     1894      -81     
==========================================
- Hits         1015      953      -62     
+ Misses        960      941      -19
Impacted Files Coverage Δ
tedana/workflows/tedana.py 14.56% <25%> (+0.38%) ⬆️
tedana/selection/_utils.py 14.28% <0%> (-0.61%) ⬇️
tedana/tests/test_utils.py 100% <0%> (ø) ⬆️
tedana/utils.py 100% <0%> (+1.86%) ⬆️
tedana/io.py 37.6% <0%> (+2.04%) ⬆️

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 98c77da...411a967. Read the comment docs.

@tsalo tsalo mentioned this pull request Feb 27, 2019
@emdupre
Copy link
Member

emdupre commented Mar 1, 2019

I compared the artifacts for the five-echo dataset for this PR and current master -- it looks like (unsurprisingly) the mask differences are rippling down to the component level.

For example, in the most recent master circle build the first component is rejected for mid-kappa thresholding, while with this PR we see that the first component is now accepted.

Just to say explicitly: I am in favor of this change (!), but it might be something we're ready with our rationale for, either noted here or in the docs.

@tsalo
Copy link
Member Author

tsalo commented Mar 1, 2019

I can definitely update the documentation in the Processing pipeline details page, the tedana_workflow documentation, and the CLI documentation.

Also, the CI run where the first component (i.e., the one with the highest Kappa) is classified as mid-Kappa seems to be from #228. That shouldn't be possible, so I'm going to dig into that over in that PR.

Edit: Sorry, that was definitely master. I am looking into it in #229.

@emdupre
Copy link
Member

emdupre commented Mar 4, 2019

Do you want to merge this, or do you want to wait pending investigations in #229 ?

@tsalo
Copy link
Member Author

tsalo commented Mar 4, 2019

I don't think anything in #229 will tie directly into the mask, so I'd be happy to merge this now.

@emdupre
Copy link
Member

emdupre commented Mar 4, 2019

Merging then 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change WIll make a non-trivial change to outputs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants