SIMEXP / load_confounds

Load fMRIprep confounds in python

MIT License

36 stars 12 forks source link

Iss78: properly handling multiple masks in anatCompCor #115

Closed pbellec closed 3 years ago

pbellec commented 3 years ago

Continuing work from https://github.com/smeisler/load_confounds/tree/iss78 ... adding a test.

pbellec commented 3 years ago

a note @smeisler code like

csf_comps = [comp for comp in compcor_cols if confounds_json[comp]["Mask"] == "CSF"

leads to a bug.

fMRIprep generates components numbered 0, ..., 9, 10, 11 etc. Which means that the 10 component comes before component 2.

So if you retain the first 10 component, you will keep component 11, 12 etc, and not component 2.

The original code is longer but easier to read, and does extract component in the right order.

pbellec commented 3 years ago

sorry, my comment above is incorrect. It does go 01, 02, etc. But the problem I described does arise with components 100, 101 etc being ordered before component 20, 21, etc. So it's still leading to a bug, not just how I described it.

codecov[bot] commented 3 years ago

Codecov Report

Merging #115 (607984b) into master (125295b) will increase coverage by 0.27%. The diff coverage is 80.59%.

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage   93.62%   93.90%   +0.27%     
==========================================
  Files           6        6              
  Lines         502      525      +23     
==========================================
+ Hits          470      493      +23     
  Misses         32       32

Impacted Files	Coverage Δ
load_confounds/parser.py	`86.80% <70.45%> (+0.85%)`	:arrow_up:
load_confounds/strategies.py	`100.00% <100.00%> (ø)`
load_confounds/tests/test_parser.py	`100.00% <100.00%> (ø)`
load_confounds/tests/test_strategies.py	`100.00% <100.00%> (ø)`

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 125295b...607984b. Read the comment docs.

pbellec commented 3 years ago

I had to refactor the code quite a bit to address the issue above, the "cognitive complexity warning". I think I managed to simplify the code quite a bit.

Notable API changes:

n_compcor now defaults to "auto" which uses all the available components. This is consistent with conventions in nilearn. None typically means "don't use", which would be confusing to users.
acompcor_mask only played a role if it was equal to combined. I have thus replaced it by a parameter acompcor_combined which is a boolen that defaults to true.

I have added some tests for the different strategies as well as the n_compcor parameter.

In terms of the warnings on the PR:

The codeclimate issue will be fixed when this PR is merged: https://github.com/SIMEXP/load_confounds/pull/95 and the issue is not related to this PR.
The coverage issue is related to censoring, which is not currently tested but is already merged in the code base.

So this is ready to go, I am merging.