-
Notifications
You must be signed in to change notification settings - Fork 13
[ENH]: Add scrubbing support in fMRIPrepConfoundRemover
#421
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
❌ Your project status has failed because the head coverage (85.72%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
==========================================
+ Coverage 85.67% 85.73% +0.05%
==========================================
Files 133 133
Lines 5633 5656 +23
Branches 954 958 +4
==========================================
+ Hits 4826 4849 +23
Misses 618 618
Partials 189 189
Flags with carried forward coverage won't be shown. Click here to find out more.
|
15bab84 to
1332737
Compare
|
|
Two comments:
I think the implementation should come from nilearn: https://nilearn.github.io/stable/modules/generated/nilearn.interfaces.fmriprep.load_confounds.html#nilearn.interfaces.fmriprep.load_confounds Basically here you can set the |
2080792 to
e392c7b
Compare
It generates a mask for indexing the time dimension and passes it to
We don't use |
I see that you use
We need to somehow "mimic" that behaviour. That's what I meant. The scrubbing mask can be either from dvars or fd. |
Do the new commits address it? |
54d8c96 to
563fd39
Compare
We are still missing the Coverage is low, meaning that we are missing tests. |
Just so that it's explicit:
I thought the columns should be in the returned dataframe?
|
|
@fraimondo would really appreciate your feedback to the previous comment. |
|
I've updated the interface and also improved the logic by replicating nilearn's implementation, would appreciate a review @fraimondo @kaurao |
fraimondo
left a comment
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.
Once CI is ready, we can merge
b0711a4 to
f2fb5e8
Compare
…n fMRIPrepConfoundRemover
f2fb5e8 to
e5ada55
Compare
This PR adds scrubbing support in
fMRIPrepConfoundRemoverby usingstd_dvarsfrom fMRIPrep output.