FIX/EHN update load_confound_strategy defaults #4225
Conversation
Closes nilearn#4222 Closes nilearn#4223 - deprecation for scrubbing thresholds to match fmriprep docmentation FD will be changed to 0.5 and std DVARS 1.5 - allow global signal in compcor strategy to reflect on fmriprep docmentation. This change does not break any existing behaviour as the defalt remains to be False Need help: depricated version?
|
👋 @htwangtw Thanks for creating a PR! Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft. Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4225 +/- ##
==========================================
- Coverage 92.07% 92.05% -0.03%
==========================================
Files 144 144
Lines 16374 16417 +43
Branches 3428 3433 +5
==========================================
+ Hits 15077 15112 +35
- Misses 756 762 +6
- Partials 541 543 +2 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| warnings.warn( | ||
| category=DeprecationWarning, | ||
| message=fd_threshold_default, | ||
| stacklevel=3, |
There was a problem hiding this comment.
Wonder if the stack level should not be 2 here so that the warning appears with where load_confounds was called.
Note: the warning stacklevel in the code base are overall wildly inconsistent, probably something we want to clean up a bit to give user better feedback.
There was a problem hiding this comment.
I tried to call the function and feel level 2 makes most sense to me. I don't know the logic behind stack level and didn't look too much into it. Happy to have it changed to whatever it seems fit
There was a problem hiding this comment.
I would say don't sweat it too much for now until we got some rule of thumb we agree on regarding how to handle this.
| @@ -37,6 +37,7 @@ | |||
| "motion": "full", | |||
| "n_compcor": "all", | |||
| "compcor": "anat_combined", | |||
| "global_signal": None, | |||
There was a problem hiding this comment.
Do we want to add this in an example?
There was a problem hiding this comment.
In fact we cannot create an example with the example dataset in the same tutorial because there's no json file for the confounds. We will have to update the example dataset to do so
There was a problem hiding this comment.
I think this the 3rd instance where we cannot show things in example because we rely on too old datasets. Probably worth opening an issue to identify what BIDS dataset we have could potentially be updated.
There was a problem hiding this comment.
Sounds good to me. I did downsampled two fMRIPrep preprocessed dataset for my other projects as test data (ds000228, ds000030).
Co-authored-by: Remi Gau <remi_gau@hotmail.com>
|
Documentation building is still broken... |
|
Thanks @Remi-Gau for fixing the docs.... |
Closes #4222
Closes #4223
deprecation for scrubbing thresholds to match fmriprep docmentation (
fd_threshold=0.5;std_dvars_threshold=1.5)for
load_confounds_strategyallow global signal in compcor strategy to reflect on fmriprep documentation. This change does not break any existing behaviour as the default remains to be FalseNeed help: