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] Derive masks to support native-space data #24

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Nov 11, 2020

References #2 and stems from Brainhack-Donostia/ica-aroma-org#49.

Changes proposed in this pull request:

  • Add function to derive masks from EPI data and CSF tissue probability map or mask.
  • Have the ICA function return the relevant ICA outputs instead of inferring the filenames in the main workflow function.
  • Use the new derived masks throughout the workflow.

@eurunuela
Copy link
Collaborator

Thank you @tsalo ! I tried running the integration tests and it fails.

I think it's related to nibabel. Could you check it out please?

@tsalo
Copy link
Member Author

tsalo commented Nov 11, 2020

I didn't get any errors in the workflow itself, but it looks like the masks are different enough that the ICA produces different components, which is causing the test to fail.

@oesteban
Copy link

I tried running the integration tests and it fails.

You guys mean locally right? Because the integration tests on Circle don't seem to have ever worked, correct?

aroma/aroma.py Outdated Show resolved Hide resolved
@tsalo
Copy link
Member Author

tsalo commented Nov 11, 2020

Right. The integration tests are going to fail on CircleCI until we get rid of FSL completely- unless we set up a Dockerfile with FSL installed, which we haven't done because we ultimately want to drop the dependency anyway.

Copy link

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

I think this is a good idea. Please check #29 which will make these kinds of refactors easier to check for regressions

aroma/features.py Outdated Show resolved Hide resolved
aroma/utils.py Outdated Show resolved Hide resolved
aroma/utils.py Show resolved Hide resolved
@eurunuela
Copy link
Collaborator

That's a weird error you got here. Is it somehow related to nilearn's load_niimg() function?

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