[FIX] Load compcor related confounds generated from fmriprep 21.x series #3285
Conversation
- refactor the test file loader - add test data and make sure the corret files are loaded - apply compcor loading suggestions from @askieslinger Still need to investigate if the actual behaviour is inline with the literature
|
👋 @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.
We will review it as quick as possible, feel free to ping us with questions if needed. |
the code should now work for both before and after 21.x update. * simply _check_compcor_method by using all five options as prefix checking keys * add tests to test_load_confounds_strategy.py::test_strategy_compcor and test_load_confounds.py::test_n_compcor to make sure the correct componenets are loaded
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3285 +/- ##
=======================================
Coverage 91.92% 91.92%
=======================================
Files 145 145
Lines 16337 16354 +17
Branches 3407 3424 +17
=======================================
+ Hits 15017 15034 +17
Misses 777 777
Partials 543 543
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @htwangtw !
I made a quick first pass and it LGTM. I just have a small suggestion to refactor a test.
Also, could you add a whatsnew ?
nilearn/interfaces/fmriprep/tests/test_load_confounds_strategy.py
Outdated
Show resolved
Hide resolved
* shorten the `test_strategy_compcor` by factorising the check on different fmriprep versions * add an entry in `latest.rst`
That's a great point. I think at this point we should only support LTS. I will have a look and perhaps a different PR to add deprecation warning about dropping non-LTS versions. |
|
@bthirion @NicolasGensollen Let me know if there's more we want to discuss about this PR! |
| @@ -42,22 +44,17 @@ def _select_compcor(compcor_cols, n_compcor): | |||
| # only select if not "all", or less components are requested than there | |||
| # actually is | |||
| if (n_compcor != "all") and (n_compcor < len(compcor_cols)): | |||
| compcor_cols = compcor_cols[0:n_compcor] | |||
| compcor_cols = compcor_cols[:n_compcor] | |||
There was a problem hiding this comment.
@htwangtw would it make sense to add a check around here? Something like a warning if no CompCor regressors are found?
There was a problem hiding this comment.
Good point - I have yet encountered this case but is it possible with how compcor is done?
I know it's possible for fMRIPrep to output confounds with no cosineXX regresssors
There was a problem hiding this comment.
I hadn't really considered that. I don't know if fMRIPrep would ever not write out compcor regressors, short of a failure within the workflow. I was more thinking that, if fMRIPrep changes things again, having warnings in these functions would make it easier for users to tell something's gone wrong.
There was a problem hiding this comment.
Right now the behaviour is the function will just fail, and a check is implemented in _load_single_confounds_file, so we don't need to write things in each function
The only variable that could be missing when fMRIPrep can run correctly is cosineXX, when the timeseries is too short.
This deserves it's own issue
There was a problem hiding this comment.
Sorry for the delay in responding. I don't think _check_error catches the missing CompCor regressors. I tried running load_confounds on an fMRIPrepped file from 22.0.0 with the following kwargs:
{'strategy': ['motion', 'high_pass', 'compcor'],
'motion': 'derivatives',
'compcor': 'anat_separated',
'n_compcor': 5}
It successfully ran, but the only regressors it returned were the cosine terms, motion parameters, and the motion derivatives. I wouldn't expect 22.x acompcor confounds to be loaded correctly per #3268, but I would have expected an error.
There was a problem hiding this comment.
No worries! I really appreciate that you took time to look into this!
I will have more time to work on this next week..
There was a problem hiding this comment.
checked on a 23.0.2 dataset
from nilearn.interfaces.fmriprep import load_confounds
nii = "/home/remi/datalad/datasets.datalad.org/openneuro-derivatives/ds003768-fmriprep/sub-01/func/sub-01_task-rest_run-1_desc-preproc_bold.nii.gz"
confounds_out, sample_mask_out = load_confounds(
nii,
strategy=["motion", "high_pass", "compcor"],
motion="derivatives",
compcor="anat_separated",
n_compcor=5,
)
print(sorted(confounds_out.columns))and got this:
['c_comp_cor_00', 'c_comp_cor_01', 'c_comp_cor_02', 'c_comp_cor_03', 'c_comp_cor_04', 'cosine00', 'cosine01', 'cosine02', 'cosine03', 'cosine04', 'cosine05', 'cosine06', 'cosine07', 'rot_x', 'rot_x_derivative1', 'rot_y', 'rot_y_derivative1', 'rot_z', 'rot_z_derivative1', 'trans_x', 'trans_x_derivative1', 'trans_y', 'trans_y_derivative1', 'trans_z', 'trans_z_derivative1', 'w_comp_cor_00', 'w_comp_cor_01', 'w_comp_cor_02', 'w_comp_cor_03', 'w_comp_cor_04']AFAICT this test also catches missing regressors:
|
@htwangtw |
It's pretty much ready and then I forgot to remind people to review it.... I can fix it up this week. |
|
@htwangtw |
|
|
Thanks a bunch @Remi-Gau ! |
|
OK will merge this. @tsalo |
Closes #3268.
_check_compcor_methodby using all five options as prefix checking keystest_load_confounds_strategy.py::test_strategy_compcorandtest_load_confounds.py::test_n_compcorto make sure the correct components are loaded for 21.x and older