[NF] migrate package `load_confounds` main function `load_confounds` #2946

htwangtw · 2021-09-07T03:17:59Z

Start migrating load_confounds to nilearn (#2777) on behalf of all coauthors.

ADD base class Confounds and all helper functions
ADD test data; taken from OpenNeuro ds000003
ADD all tests associated with class Confounds

Few things still needs some discussions and/or changes:

codecov · 2021-09-07T03:25:36Z

Codecov Report

Merging #2946 (6ff1f1b) into main (f8a9c23) will increase coverage by 0.46%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2946      +/-   ##
==========================================
+ Coverage   89.31%   89.78%   +0.46%     
==========================================
  Files         100      105       +5     
  Lines       13912    14256     +344     
  Branches     2711     2781      +70     
==========================================
+ Hits        12426    12800     +374     
+ Misses        896      869      -27     
+ Partials      590      587       -3

Impacted Files	Coverage Δ
nilearn/__init__.py	`83.87% <ø> (ø)`
nilearn/_utils/fmriprep_confounds.py	`100.00% <100.00%> (ø)`
nilearn/input_data/__init__.py	`100.00% <100.00%> (ø)`
nilearn/input_data/fmriprep_confounds.py	`100.00% <100.00%> (ø)`
nilearn/input_data/fmriprep_confounds_compcor.py	`100.00% <100.00%> (ø)`
...ilearn/input_data/fmriprep_confounds_components.py	`100.00% <100.00%> (ø)`
nilearn/input_data/fmriprep_confounds_scrub.py	`100.00% <100.00%> (ø)`
nilearn/input_data/fmriprep_confounds_utils.py	`100.00% <100.00%> (ø)`
nilearn/regions/rena_clustering.py	`95.00% <0.00%> (-0.60%)`	⬇️
nilearn/glm/thresholding.py	`95.69% <0.00%> (-0.38%)`	⬇️
... and 26 more

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 f8a9c23...6ff1f1b. Read the comment docs.

htwangtw · 2021-09-07T14:58:25Z

I have done some research on the minimal version of fMRIprep output we can support and added them to the existing docstrings. We can fully support everything from 1.4.X, and 1.2.X except compcor.

There are two aspect that I would like @NicolasGensollen to make an executive call on:

I can make sure some sanity checks on versions added to this PR, or as a separate one. I prefer to have it as a separate one but would like to see what you think makes more sense.
Name of the module: load_confounds only works with fMRIprep outputs. The current name is potentially confusing. It can make user think this is for all kinds of confounds, rather than just for fMRIprep.
My idea is to put load_confounds under a directory called fmriprep, and import the Confounds like this:

from nilearn.fmriprep import Confounds

This can be a place for all potential future fmriprep helper functions. WDYT? cc @pbellec

tsalo · 2021-09-07T15:31:15Z

@htwangtw can the module work generally with BIDS derivatives (especially w.r.t. bids-standard/bids-specification#519, which proposes rules that are probably what dictate fMRIPrep's derivatives)? I think a nilearn.bids would be preferable to a nilearn.fmriprep.

htwangtw · 2021-09-07T15:45:40Z

@tsalo Thanks for bringing this up!

I had a quick read through of BEP 012. From what I am aware, load_confounds fits well, as there is plan to adapt to new changes in fMRIprep. We have some of the very minimal file validity checks for now and it would be great to have it streamlined with BIDS in the future. nilearn.bids is a good idea.

pbellec · 2021-09-07T15:54:55Z

I feel like nilearn.fmriprep would be the most straightforward at the moment, and something that may need to be revisited if a
more general set of bids helper gets developed for nilearn.

Currently, the main use case for the confounds features is for fMRIprep users. I am not aware of software using the BIDS fMRI derivatives at the moment, and I am not sure that fMRIprep itself conforms to the standard, which is still a draft as far as I can tell. Also, BIDS is most commonly associated with raw data, and unless nilearn expands to offer tools to handle BIDS data (which would then overlap with pybids), I am not sure what other modules will live in nilearn.bids.

tsalo · 2021-09-07T15:58:52Z

@pbellec one motivation I have for bids over fmriprep is that I'm working on functions to write out BIDS derivatives for GLMs (also based on a draft instead of the real specification) in #2715. We've also discussed the need for lightweight BIDS querying tools so that nilearn doesn't need pybids as a dependency. Some of those tools may end up in nilearn (or pybids-light, which could become a dependency).

pbellec · 2021-09-07T16:31:33Z

so if there is a push for a set of bids tools in nilearn, nilearn.bids sounds good. Then we may need to rename the strategy module something like fmriprep_denoise.py and expose the confound strategies directly through nilearn.bids. So users would type

from nilearn.bids import Scrubbing

Another option would be to have long and explicit class names, like what is done for the masker classes.

from nilearn.bids import FmriPrepScrubbing

NicolasGensollen · 2021-09-07T16:52:44Z

Thanks for opening this @htwangtw ! 👍
I'll make a review tomorrow but it looks really good at first glance.
I think it could be helpful to have some examples that we could use to test things a little bit (they will be useful for the docs at some point anyway).
Concerning the name of the module, I think it should be explicit enough such that users aren't surprised by what is inside (we get this remark every now and then about input_data which could have been maskers for example).
I'm also worried that if we have a bids module, then it would make sense to put all bids-related functionalities inside which could quickly get messy (not saying it would necessarily, just trying to think how it could go wrong...).

htwangtw · 2021-09-07T17:30:03Z

I can certainly add an examples. We do have an existing demo jupyter notebook based on a NiftiMasker example. It needs some updates but I can certainly put it somewhere.

bthirion

Thx for this PR !
Now I better understand why the code refers to fMRIprep rather than BIDS.
What i don't understand atm is why we need a Confound class, not a function ? (functions are earsier to maintain).

nilearn/load_confounds/confounds.py

nilearn/load_confounds/parser.py

nilearn/load_confounds/data/missing_desc-confounds_regressors.tsv

NicolasGensollen

Thanks @htwangtw ! 👍
I made a first pass (mostly trying to understand how things are structured) and made a few small comments (see below).

examples/03_connectivity/plot_load_confounds.py

nilearn/__init__.py

nilearn/load_confounds/__init__.py

nilearn/load_confounds/parser.py

nilearn/load_confounds/tests/test_parser.py

htwangtw · 2021-09-08T17:49:11Z

w.r.t. fmriprep vs BIDS -
I had a think about this today. For now we are happy with what load_confounds is doing - it's a one stop shop for users to use fMRIprep outputs in nilearn. With the predefined strategies, it would be even simpler. Thus I have a preference for my original proposal (fmriprep).

Moving load_confounds to part of BIDS integration can be some work down the line. It will be a while before BEP 012 get merged and applied to fMRIPrep. That's a lot of things to anticipate and address for now, and I cannot see the changes of BEP 012 coming in the immediate future. At the moment load_confounds is for fMRIprep rather than BIDS.

NicolasGensollen

Thanks for addressing the reviews so quickly @htwangtw !
Below are a few comments and suggestions (mostly on documentation and formatting).

doc/modules/reference.rst

nilearn/load_confounds/__init__.py

nilearn/load_confounds/confounds.py

nilearn/load_confounds/parser.py

NicolasGensollen

@htwangtw I spent some time building the docs from your branch in strict mode which always fail because of annoying little things like wrong indents, missing blank lines and so on...
Below are a few things I had to fix to make it work as well as some suggestions for better linking of resources.
Note: Do not hesitate to request full builds from time to time by adding "[circle full]" in your commit message.

doc/modules/reference.rst

examples/03_connectivity/plot_signal_extraction.py

nilearn/load_confounds/parser.py

htwangtw · 2021-09-09T19:06:58Z

Regarding the suggestion on adding tests to private functions, I am only going to do a selective set.
We have a full coverage in the original load_confounds repo through test the high level API that's exposed to the users.
I can think of a few things that can be benefited from dedicated unit test

some edge cases for scrubbing ineracting with non-steady-state outliers (this is done)
compcor option parsing. This is anticipating the changes in fMRIprep on the new anatomical compcor label in 21.0.0rc0
Other than that I think the existing tests are doing good.

NicolasGensollen · 2021-09-10T10:10:08Z

@htwangtw I just pushed a commit to your PR in which I propose to refactor a little bit some tests.
Let me know what you think, and feel free to delete it if you are not satisfied with the proposed changes.

htwangtw · 2021-09-10T12:33:50Z

@htwangtw I just pushed a commit to your PR in which I propose to refactor a little bit some tests.
Let me know what you think, and feel free to delete it if you are not satisfied with the proposed changes.

Thanks for the refactoring @NicolasGensollen
Parametrising tests made the test more concise. I like it.

examples/03_connectivity/plot_signal_extraction.py

nilearn/_utils/load_confounds.py

nilearn/load_confounds/confounds.py

NicolasGensollen · 2021-09-14T14:23:29Z

@htwangtw The documentation build error wasn't related to this PR. I pushed a fix earlier today, so if you merge master you should have the CI all green again. ✔️

Concerning the design choice of having a class vs. functions for Confounds, I also feel like we don't really need the complexity of multiple classes inheriting from the Confounds base class as they seem to "only" redefine some of their attributes. I might be wrong, but it seems to me that they mostly provide handy constructors but do not implement additional specific logic.

Maybe the best way to decide is to compare the two designs (I can have a go at refactoring this if you want).
WDYT?

htwangtw · 2021-09-14T17:55:12Z

@htwangtw The documentation build error wasn't related to this PR. I pushed a fix earlier today, so if you merge master you should have the CI all green again. heavy_check_mark

Concerning the design choice of having a class vs. functions for Confounds, I also feel like we don't really need the complexity of multiple classes inheriting from the Confounds base class as they seem to "only" redefine some of their attributes. I might be wrong, but it seems to me that they mostly provide handy constructors but do not implement additional specific logic.

Maybe the best way to decide is to compare the two designs (I can have a go at refactoring this if you want).
WDYT?

Looks like there's problem with full build still, but partial build is fine.

Great timing! I was leaving this comment to the last. I have time and just started a local branch for the refectoring and will see how it goes.

doc/modules/reference.rst

bthirion · 2021-09-14T20:24:36Z

@htwangtw The documentation build error wasn't related to this PR. I pushed a fix earlier today, so if you merge master you should have the CI all green again. heavy_check_mark

Concerning the design choice of having a class vs. functions for Confounds, I also feel like we don't really need the complexity of multiple classes inheriting from the Confounds base class as they seem to "only" redefine some of their attributes. I might be wrong, but it seems to me that they mostly provide handy constructors but do not implement additional specific logic.

Maybe the best way to decide is to compare the two designs (I can have a go at refactoring this if you want).
WDYT?

Could not agree more. Thx !

htwangtw · 2021-09-14T20:55:54Z

@NicolasGensollen @bthirion I have refactored the code to function - was less work then I thought and not that much chance in the number of lines.
Confounds is now replaced byload_confounds.
Currently, only the function exposed to the user is load_confounds. I tried to get some of the function name and docs terminology more consistent. However I still find the module names a bit chaotic. For now I have:

from nilearn.load_confounds import load_confounds

Is this okay?
Please let me know if things are making sense!

NicolasGensollen

Thanks for refactoring this so quickly @htwangtw ! 👍
I just made a quick pass (see below), I will have another look later today.

examples/03_connectivity/plot_signal_extraction.py

nilearn/load_confounds/parser.py

NicolasGensollen · 2021-09-15T07:47:41Z

Is this okay?
Please let me know if things are making sense!

The import will depend on the name we end up choosing but from nilearn.load_confounds import load_confounds makes sense to me.

Having only load_confounds public for now also makes sense.
I believe other classes which were inheriting from Confounds will then become public functions calling load_confounds with some combinations of parameters, right?

htwangtw · 2021-09-15T15:12:36Z

I believe other classes which were inheriting from Confounds will then become public functions calling load_confounds with some combinations of parameters, right?

Yes. So it will be like:

from nilearn.load_confounds import minimal

Function:

from nilearn.load_confounds import load_confounds

def minimal(img_files, motion="full", wm_csf="basic", demean=True):
    strategy = ["high_pass", "motion", "wm_csf", "non_steady_state"]
    # some sanity checks here 
    return load_confounds(strategy=strategy, motion=motion, wm_csf=wm_csf, demean=demean)

GaelVaroquaux · 2021-09-15T15:16:04Z

In terms of import path, I wonder whether it would not make sense put this together with the maskers. I don't really like the name of the modules where the maskers are based (I'm to blame). Maybe an "io" module that groups the confounder loading and the maskers? My 2 cents from the peanut gallery

Numpy retires the legacy random state after v1.21, which only supports python 3.7 and above

htwangtw · 2021-09-28T21:43:41Z

All builds passed 🎉 I will implement the four preset strategy next.
cc'd @pbellec

NicolasGensollen

Thanks @htwangtw !
Just a minor formatting suggestion but LGTM otherwise.

nilearn/input_data/fmriprep_confounds.py

Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

NicolasGensollen

Thanks @htwangtw ! 👍
I believe this is ready to merge. I think we can wait a day or two to give a chance for more reviews.
Has anyone planned to make another review? (most likely @bthirion)

bthirion · 2021-09-30T14:12:00Z

Will do asap...

bthirion

Hi, I've simply foud a couple of typos. This good to go otherwise !
Thx again,

examples/03_connectivity/plot_signal_extraction.py

nilearn/_utils/fmriprep_confounds.py

nilearn/input_data/fmriprep_confounds_scrub.py

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

`srub` now accept an integer from 0 to n. After removing high motion volumes, segments with number of frame < `scrub` will also be removed. Default value is 5. ADD explicit author names in th main fmriprep_confounds module.

nilearn/input_data/fmriprep_confounds_scrub.py

bthirion

LGTM, thx !

NicolasGensollen

LGTM, thanks @htwangtw !

NicolasGensollen · 2021-10-12T08:03:15Z

Thanks everyone for the work on this! 💯
Merging... 🎉

pbellec · 2021-10-12T13:16:10Z

Congrats @htwangtw for driving this PR home, and big thanks to the load_confounds contributors and the nilearn team for working with us on the code base, especially @NicolasGensollen and @bthirion. I hope this feature will be valuable to the community.

bthirion · 2021-10-12T17:07:54Z

That's a great new feature indeed !

Closes nilearn#2946. This is the second part to merge pacakge `load_confounds`. * Add predefined strategy with a dictionary containing relevant parameters * New function fmriprep_confounds_strategy to parse user input for the predefined strategy * Add example and brief explaination of the four based strategy The example is super preliminary, please raise any suggestion and better way to introduce the function. Co-authored-by: Pierre Bellec <pierre.bellec@gmail.com>

* [NF] get fmriprep confounds with fixed strategy Closes #2946. This is the second part to merge pacakge `load_confounds`. * Add predefined strategy with a dictionary containing relevant parameters * New function fmriprep_confounds_strategy to parse user input for the predefined strategy * Add example and brief explaination of the four based strategy The example is super preliminary, please raise any suggestion and better way to introduce the function. Co-authored-by: Pierre Bellec <pierre.bellec@gmail.com> * LINT pep8 * DOCS limit of example dataset Some confound variables are not availible in the reduced experimental dataset Comment out the lines for now to let the test pass * Typos Co-authored-by: bthirion <bertrand.thirion@inria.fr> * Apply suggestions from code review Co-authored-by: Gensollen <nicolas.gensollen@gmail.com> * EHN raise error for invalid strategy Remove examples that cannot be executed correctly * LINT flake8 * Apply suggestions from code review Co-authored-by: Gensollen <nicolas.gensollen@gmail.com> * [DOCS] maybe demostrate the results of denoising through PCA * [TEST] factorise checks on preset strategy defaults * [DOCS] add a table to summarise the strategies * [DOCS] skip the table for pep8 * [FIX] remove irrelevant stuff added by my IDE * [DOCS]syntax error when referencing the functions * [FIX] simple strategy motion default should be 'full' * [DOC] fix the broken example due to change in default setting and limit of the example dataset * DOCS move strategy description from example to the function docstring * [circle-full] Add descriptions of fmriprep_confounds in the user guide * Apply suggestions from code review Co-authored-by: Gensollen <nicolas.gensollen@gmail.com> Co-authored-by: bthirion <bertrand.thirion@inria.fr> * [circle full] Fix the table formatting * DOCS fix sections in examples * [circle full] Trigger full build Co-authored-by: Pierre Bellec <pierre.bellec@gmail.com> Co-authored-by: bthirion <bertrand.thirion@inria.fr> Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

bthirion reviewed Sep 7, 2021

View reviewed changes

NicolasGensollen reviewed Sep 8, 2021

View reviewed changes

NicolasGensollen reviewed Sep 9, 2021

View reviewed changes

htwangtw closed this Sep 9, 2021

htwangtw reopened this Sep 9, 2021

bthirion reviewed Sep 12, 2021

View reviewed changes

tsalo reviewed Sep 14, 2021

View reviewed changes

doc/modules/reference.rst Outdated Show resolved Hide resolved

NicolasGensollen reviewed Sep 15, 2021

View reviewed changes

examples/03_connectivity/plot_signal_extraction.py Outdated Show resolved Hide resolved

nilearn/load_confounds/parser.py Outdated Show resolved Hide resolved

htwangtw changed the title ~~[NF] migrate load_confounds base class~~ [NF] migrate package load_confounds main function load_confounds Sep 15, 2021

htwangtw added 5 commits September 28, 2021 09:32

TEST increase DoF and add randome seed to improve stability

8787d97

[circle full] Add what's new entry

939ac4b

[circle full] typo

bbdf73f

[circle full] fall back to random seed in test for version <3.7

edb77f3

Numpy retires the legacy random state after v1.21, which only supports python 3.7 and above

[circle full] typo in reference to NifitMasker

a8727bd

NicolasGensollen reviewed Sep 29, 2021

View reviewed changes

nilearn/input_data/fmriprep_confounds.py Outdated Show resolved Hide resolved

[circle full] Update nilearn/input_data/fmriprep_confounds.py

764de48

Co-authored-by: Gensollen <nicolas.gensollen@gmail.com>

htwangtw mentioned this pull request Sep 29, 2021

Prevent user passing standardize_confounds=False with confound variables to signal.clean #2999

Closed

NicolasGensollen approved these changes Sep 30, 2021

View reviewed changes

htwangtw mentioned this pull request Oct 6, 2021

Integration of load_confounds to nilearn #2777

Closed

bthirion reviewed Oct 11, 2021

View reviewed changes

tsalo mentioned this pull request Oct 11, 2021

A proposal to reorganize Maskers and I/O functions #3014

Closed

htwangtw and others added 2 commits October 11, 2021 16:04

Apply suggestions from code review

c1c8454

Co-authored-by: bthirion <bertrand.thirion@inria.fr>

[circle full] [ENH] expose full scrubbing segment length

6c86349

`srub` now accept an integer from 0 to n. After removing high motion volumes, segments with number of frame < `scrub` will also be removed. Default value is 5. ADD explicit author names in th main fmriprep_confounds module.

bthirion reviewed Oct 11, 2021

View reviewed changes

nilearn/input_data/fmriprep_confounds_scrub.py Outdated Show resolved Hide resolved

htwangtw added 3 commits October 11, 2021 18:35

[circle full] fix example to fit the new srub parameter

c24b0c9

[circle full] default value is not useful in private function

461fae8

[circle full] fix scrubbing related tests; add doctring

6ff1f1b

bthirion approved these changes Oct 12, 2021

View reviewed changes

NicolasGensollen approved these changes Oct 12, 2021

View reviewed changes

NicolasGensollen merged commit dd16696 into nilearn:main Oct 12, 2021

htwangtw mentioned this pull request Oct 12, 2021

[NF] get fmriprep confounds with fixed strategy #3016

Merged

htwangtw deleted the load_confounds_baseclass branch January 26, 2022 15:08

[NF] migrate package load_confounds main function load_confounds #2946

[NF] migrate package load_confounds main function load_confounds #2946

Conversation

htwangtw commented Sep 7, 2021 • edited

codecov bot commented Sep 7, 2021 • edited

Codecov Report

htwangtw commented Sep 7, 2021

tsalo commented Sep 7, 2021

htwangtw commented Sep 7, 2021

pbellec commented Sep 7, 2021

tsalo commented Sep 7, 2021

pbellec commented Sep 7, 2021

NicolasGensollen commented Sep 7, 2021

htwangtw commented Sep 7, 2021

bthirion left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

htwangtw commented Sep 8, 2021

NicolasGensollen left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

htwangtw commented Sep 9, 2021

NicolasGensollen commented Sep 10, 2021

htwangtw commented Sep 10, 2021

NicolasGensollen commented Sep 14, 2021

htwangtw commented Sep 14, 2021

bthirion commented Sep 14, 2021

htwangtw commented Sep 14, 2021

NicolasGensollen left a comment

Choose a reason for hiding this comment

NicolasGensollen commented Sep 15, 2021

htwangtw commented Sep 15, 2021

GaelVaroquaux commented Sep 15, 2021 via email

htwangtw commented Sep 28, 2021

NicolasGensollen left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

bthirion commented Sep 30, 2021

bthirion left a comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

NicolasGensollen commented Oct 12, 2021

pbellec commented Oct 12, 2021

bthirion commented Oct 12, 2021

[NF] migrate package `load_confounds` main function `load_confounds` #2946

[NF] migrate package `load_confounds` main function `load_confounds` #2946

htwangtw commented Sep 7, 2021 •

edited

codecov bot commented Sep 7, 2021 •

edited