simexp / load_confounds Goto Github PK

the tip of master is not version 0.5 anymore, this should be reflected in the version number.

to be consistent with n_compcor, pca_motion should be renamed n_motion, and should have the following behaviour
n_motion=0 (default) no PCA is applied
0 < n_motion < 1 select the number of components by the percentage of explained variance.
1 < n_motion directly select the number of compontents (but select at most the 6, 12 or 24 available in the model, with a warning if the number specified by the user is higher than that).

https://pypi.org/project/load-confounds/

We're going to be going through and conforming fMRIPrep derivatives to the derivatives spec soon, so it's possible that derivatives may change. It might be useful to have some functionality to switch strategies based on fMRIPrep version, if that doesn't alrleady exist.

because the confounds list in fmriprep changes across versions, it could be that a strategy requires confounds that are absent in the file.

This should raise an error.

it seems to be selecting very few components

The tests currently are a long list of variations on the same snippet, combining certain parameters. It would be possible to replace all the test by one nested for loop, with a list of parameters to be tested, and a list of expected confound labels associated with each parameter.

we want people to feel welcome proposing changes. The tedana project has a great contributing file
https://github.com/ME-ICA/tedana/blob/master/CONTRIBUTING.md

Consider adapting it for fmriprep_load_confounds. Simplify a bit maybe though, because this project is pretty small, and nowhere as ambitious as tedana.

it's not clear how to deal with high_pass vs low_pass. At the moment we are only working from an example with high_pass regressors, but those should be controlled separately.

I think we should use the same name for the library and the main method. I like load_confounds much better.

I don't think it makes sense to have a __main__ method as load_confounds is designed to load data in a python environment, and not be executed through the command line.

tests currently check that the regressors are properly filtered, but not that they can be fed to nilearn.

Many confounds are only calculated optionnally by fmriprep. It would be useful to have a file with all available options for testing, and adding new supported denoising strategies such as aroma.

I suspect there is still data saved in there.
Would be useful to "doctor" the git history to get rid of this.

This section of the code is used to filter different regressors from the raw confounds:
https://github.com/SIMEXP/load_confounds/blob/master/load_confounds.py#L9-L31

It is however not straightforward to understand how this dictionary is structured, and unless you parse the rest of the code it is very hard to follow what is going on.

To increase readability it may be better to have dedicated functions like

_load_motion
_load_high_pass
_load_masks

and then a separate function to establish which components to add in the model based on the strategy and a few flags.

this would be implemented just like scrubbing.

See this issue:
courtois-neuromod/cneuromod_docs#23

Instead of introducing a new method load_confounds_multi, it would be more elegant to test if the tsv file provided by the user is a string (in which case a single confound array is loaded) or an array (in which case the method would iterate on all inputs).

when working with nilearn, it will often be the case that users have built a list of nifti file names they want to load. It would be useful to let them pass such list directly to the confound loader. As fmriprep uses a systematic naming schemes, we could automatically search for the corresponding confound files. We may also support niftiimage object, which carry the file name in their attributes. https://nilearn.github.io/manipulating_images/input_output.html

This way users would simply need to pass their list of imgs to get back the confounds arrays, ready to be fed in a NiftiMasker or any nilearn function with a confound argument.

as the name on pip is load_confounds it would be easier to use it for the repo name too.

First the name is not really self-explanatory. Maybe _get_confounds_columns as this is literally what it does.

Second I suggest to have a single line of documentation, as this is a private function.
The parameters are redundant with other functions and do not need to be documented again here.

"""Get a list of all confound columns that fit the strategy."""

Third, the code uses an in-line double for loop with an if statement.
https://github.com/SIMEXP/load_confounds/blob/master/load_confounds.py#L51-L57
That is hard to read, and not compact.
I suggest using a regular double for loop with an if statement.
Also add a comment to explain what is being done here: the function grabs all columns in the raw confounds that contains a string from the confounds dictionary.

We should try to replicate the strategies found in this paper:
https://www.sciencedirect.com/science/article/pii/S1053811917310972

update setup.py too

some procedures (eg nilearn's group dictionary learning) require to pass a list of datasets. It would be useful to support such a list, and return a list of confound arrays.

the aim of this private function is to find the confounds associated with a particular imaging dataset. The name should reflect that. Maybe _get_confounds_files ?

The loader says 6params ??

from what I see, the library uses only pandas and sklearn.
But requirements.txt list lots of different dependencies.

we should simulate a 4D dataset with known confounds, and check that nilearn indeed removes the confounds.

the documentation of that parameter currently lives in a private function. It should be moved and integrated in the documentation of the main load_confounds function.

Also, the name of that parameter is not explicit enoough.
It is the number of motion components, so it should be called n_motion rather than n_component

The default is currently 0.95 but PCA reduction of motion parameters is not a standard preprocessing strategy. It should be set at 0 by default, i.e. use all the motion regressors.

Of the strategies currently listed, only "minimal" is a true preprocessing strategy.
The others are "private" strategies, used internally to assemble more complex strategies.
They should not be listed in the documentation of the main public method.

this line will only work for some options in the fmriprep pipeline:
https://github.com/SIMEXP/load_confounds/blob/master/load_confounds.py#L212

It should be replaced by a general pattern, e.g. scrapping everything after _space-