[NF] get fmriprep confounds with fixed strategy #3016

htwangtw · 2021-10-12T21:02:19Z

Closes #2777.

This is the second part to merge package load_confounds. Based on the discussion discussed at the office hour:

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 explanation of the four based strategy

The example is super preliminary, please raise any suggestion and better way to introduce the function.
Still I am looking for suggestion to document the dictionary with nilearn.input_data.fmriprep_confounds parameters so it would be easier for maintenance.

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>

github-actions · 2021-10-12T21:02:35Z

👋 @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.

PR has an interpretable title.
PR links to Github issue with mention "Closes #XXXX"
Code is PEP8-compliant.
(Bug fixes) There is at least one test that would fail under the original bug conditions.
(New features) There is at least one unit test per new function / class.
(New features) The new feature is demoed in at least one relevant example.

We will review it as quick as possible, feel free to ping us with questions if needed.

codecov · 2021-10-12T21:09:44Z

Codecov Report

Merging #3016 (722d2f1) into main (e9c4392) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3016      +/-   ##
==========================================
+ Coverage   90.05%   90.07%   +0.02%     
==========================================
  Files         105      106       +1     
  Lines       14332    14361      +29     
  Branches     2787     2794       +7     
==========================================
+ Hits        12906    12935      +29     
  Misses        848      848              
  Partials      578      578

Impacted Files	Coverage Δ
nilearn/input_data/fmriprep_confounds.py	`100.00% <ø> (ø)`
nilearn/input_data/__init__.py	`100.00% <100.00%> (ø)`
nilearn/input_data/fmriprep_confounds_strategy.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 e9c4392...722d2f1. Read the comment docs.

Some confound variables are not availible in the reduced experimental dataset Comment out the lines for now to let the test pass

bthirion

Thx. I just found a set of typos.

examples/03_connectivity/plot_signal_extraction.py

nilearn/input_data/fmriprep_confounds_strategy.py

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

NicolasGensollen

Thanks @htwangtw ! 🚀
I made a first pass and it looked really good. 💯
I just made a few small typo comments below.
I'll have a closer look in the coming days. I think the main remaining challenge is to find a good spot for the detailed documentation on the preset strategies as we discussed on Monday. I don't think having them in a specific example will make them easy to find for users. Having them in the Notes section of the docstring is a possibility despite the additional length. Note that if we wish to use the descriptions in multiple locations, we could create entries in the standard docs (in nilearn/_utils/docs.py).

examples/03_connectivity/plot_signal_extraction.py

nilearn/input_data/fmriprep_confounds_strategy.py

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

Remove examples that cannot be executed correctly

nilearn/input_data/tests/test_fmriprep_confounds_strategy.py

NicolasGensollen

Thanks @htwangtw !
I made a few more suggestions (see comments below). Still unsure where to put the strategies descriptions though...

examples/03_connectivity/plot_signal_extraction.py

NicolasGensollen · 2021-10-15T09:37:27Z

examples/03_connectivity/plot_signal_extraction.py

+# benchmarks studies (:footcite:`Ciric2017`, :footcite:`Parker2018`), we
+# provide four preset strategies: `simple`, `scrubbing`, `compcor`, and
+# `ica_aroma`.
+#   - `simple`: high pass filtering, basic motion, basic white matter, basic


I'm wondering whether having also a table somewhere in the docs would make sense. 🤔
I'm thinking about something similar to what you have in the README of load_confounds. I think it makes it easier to spot the differences between the strategies (at least for me it is). WDYT?

I have added a table and cut the text description to the pros and cons of the method

NicolasGensollen · 2021-10-15T09:40:30Z

examples/03_connectivity/plot_signal_extraction.py

+                                                     denoise_strategy="simple")
+
+# add optional parameter global siganl
+confounds, sample_mask = fmriprep_confounds_strategy(fmri_filenames,


Right now it feels a little bit weird to have the example ending up like this. Do you have any idea of what we could show here? I would naively expect some comparison between the strategies but this might be complicated to do. WDYT?

My initial thought is to just plot connectomes cleaned with different set of confound variables.
However, aside from comparing no cleaning vs cleaned, the difference of the rest is not so easy to see by eyes.
I do have a bigger project to do a comprehensive bench mark comparison.
One thing we can do is to replicate only part of the Ciric 2017 study with this test dataset, and I already have the code

Ideally, you want to have a very short example that illustrates the impact of the deconfounding strategy. Of course, this is hard in the absence of a ground truth, but you might either spot an interpretable difference or show an impact on statistical analysis ---but we don't want do add another dataset in the examples...

if we are just plotting the deconfound connectomes, it would be redundant with the example of nilearn.input_data.fmriprep_confounds. One idea I have is to modify the FastICA example on resting state. It uses the same example as this one, and we can show the impact before and after denoising really clearly comparing the first components.

The first one is the first component of the original example, picking up lots of the frontal and occipital part that's mostly classified as noise

This one is after denoising with simple plus global signal regression, where you can see clear default mode

WDYT?

Looks good, thx ! Note simply that with ICA, the "first" components has nothing special. There is no components ordering.

Yes you are right - I can plot a few and show the proportion of noise vs meaningful networks are there

nilearn/input_data/fmriprep_confounds_strategy.py

nilearn/input_data/tests/test_fmriprep_confounds_strategy.py

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

…earn into fmriprep_confounds_strategy

htwangtw · 2021-10-20T19:38:14Z

I think the remaining issue is still where to put the docs.

So for the example I fall back to plotting the connectomes for now.
One idea I have is to run a PCA on denoised image and compare if the first component correlate with motion more without deconfounding. Plotting the weight of the first component might be helpful but I don't see that more meaningful then a actual correlation.

And thanks to the suggestion of summarising the strategy as a table, I spotted a difference in the simple strategy comparing to what load_confounds originally implemented.

…it of the example dataset

NicolasGensollen · 2021-10-21T16:17:17Z

examples/03_connectivity/plot_signal_extraction.py

+# | ica_aroma | True      | N/A    | basic  | optional      | N/A   | N/A       | N/A              | N/A           | N/A       | full      | True   | # noqa
+# +-----------+-----------+--------+--------+---------------+-------+-----------+------------------+---------------+-----------+-----------+--------+ # noqa
+#
+#   - `simple`: This approach is commonly used in resting state functional


After thinking about it, I don't believe that having the description of the strategies within the docstring of fmriprep_confounds_strategy is problematic since it's not that long in the end. (I think we can also add the table comparing strategies in the Notes section).
Actually, this is where I would expect to find this information once I have heard of fmriprep_confounds_strategy.

So I don't think we need so many details on each strategies as part of this example if this is already documented in the API docs.

However, I think we should try to "advertise" for fmriprep_confounds and fmriprep_confounds_strategy somewhere in the user guide such that users get to learn about their existence.

I'm not really sure where this would fit best, but here are two possible options:

https://nilearn.github.io/stable/connectivity/functional_connectomes.html#extracting-signals-on-a-parcellation

https://nilearn.github.io/stable/manipulating_images/masker_objects.html#temporal-filtering-and-confound-removal

WDYT? @htwangtw @bthirion

Sounds like a good idea for me to include the description in the API docs
For the user guide, I think the temporal filtering guide is more general and functional connectivity is good for showing direct impact through graphs. I incline to show case the details in the functional connectivity section, and point to the temporal filtering guide to explain how it's done under the hood. WDYT?

bthirion

Looks almost ready to go !

examples/03_connectivity/plot_signal_extraction.py

bthirion · 2021-10-21T06:48:30Z

examples/03_connectivity/plot_signal_extraction.py

+# +-----------+-----------+--------+--------+---------------+-------+-----------+------------------+---------------+-----------+-----------+--------+ # noqa
+# | ica_aroma | True      | N/A    | basic  | optional      | N/A   | N/A       | N/A              | N/A           | N/A       | full      | True   | # noqa
+# +-----------+-----------+--------+--------+---------------+-------+-----------+------------------+---------------+-----------+-----------+--------+ # noqa
+#


Thetable does not render well in the example.This better goes into the narrative documentation ?

nilearn/input_data/fmriprep_confounds_strategy.py

NicolasGensollen

Looking good! 👍
We just have to fix the last rendering issues and this should be good.

nilearn/input_data/fmriprep_confounds_strategy.py

examples/03_connectivity/plot_signal_extraction.py

doc/connectivity/functional_connectomes.rst

examples/03_connectivity/plot_signal_extraction.py

Co-authored-by: Gensollen <nicolas.gensollen@gmail.com> Co-authored-by: bthirion <bertrand.thirion@inria.fr>

bthirion

I'm happy with the current version. Thx !

NicolasGensollen

Thanks @htwangtw !
LGTM minus the small formatting point below.
Could you also add a whatsnew entry?

examples/03_connectivity/plot_signal_extraction.py

NicolasGensollen

LGTM, thanks a lot @htwangtw !

NicolasGensollen · 2021-10-28T13:07:08Z

@emdupre I was wondering whether you wanted to make another review pass on this PR.

emdupre · 2021-10-28T14:32:49Z

@emdupre I was wondering whether you wanted to make another review pass on this PR.

Unfortunately I haven't had time to do an in-depth review of this PR -- I had just noticed a handful of relative imports when it popped up in my notifications. I'm happy to do a real review, but if folks are now happy with this (and the relative imports were successfully converted to absolute imports 😸 ) then I don't want to hold this up !

NicolasGensollen · 2021-10-29T05:57:26Z

Unfortunately I haven't had time to do an in-depth review of this PR

No problem, I was just checking as you were listed among the reviewers.
I'm going to merge it then as we have already two approvals! 🎉
Thanks again @htwangtw for all the hard work on these migration PRs! 💯

LINT pep8

a7d881b

DOCS limit of example dataset

820e3e9

Some confound variables are not availible in the reduced experimental dataset Comment out the lines for now to let the test pass

bthirion reviewed Oct 13, 2021

View reviewed changes

Typos

f4a3750

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

NicolasGensollen reviewed Oct 14, 2021

View reviewed changes

htwangtw and others added 3 commits October 14, 2021 10:52

Apply suggestions from code review

6a999e0

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

EHN raise error for invalid strategy

de2d885

Remove examples that cannot be executed correctly

LINT flake8

04df339

emdupre reviewed Oct 14, 2021

View reviewed changes

nilearn/input_data/tests/test_fmriprep_confounds_strategy.py Outdated Show resolved Hide resolved

NicolasGensollen reviewed Oct 15, 2021

View reviewed changes

htwangtw and others added 9 commits October 15, 2021 13:44

Apply suggestions from code review

04f1b47

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

[DOCS] maybe demostrate the results of denoising through PCA

cb712ec

Merge branch 'nilearn:main' into fmriprep_confounds_strategy

4086d78

[TEST] factorise checks on preset strategy defaults

0bfe23f

Merge branch 'fmriprep_confounds_strategy' of github.com:htwangtw/nil…

fb34488

…earn into fmriprep_confounds_strategy

[DOCS] add a table to summarise the strategies

f22d81c

[DOCS] skip the table for pep8

044e543

[FIX] remove irrelevant stuff added by my IDE

f2a7026

[DOCS]syntax error when referencing the functions

e39595d

htwangtw added 2 commits October 20, 2021 16:11

[FIX] simple strategy motion default should be 'full'

580cd38

[DOC] fix the broken example due to change in default setting and lim…

35b7757

…it of the example dataset

NicolasGensollen reviewed Oct 21, 2021

View reviewed changes

htwangtw added 2 commits October 22, 2021 17:56

DOCS move strategy description from example to the function docstring

7bcfeb4

[circle-full] Add descriptions of fmriprep_confounds in the user guide

9668dbc

bthirion reviewed Oct 24, 2021

View reviewed changes

NicolasGensollen reviewed Oct 25, 2021

View reviewed changes

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

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

NicolasGensollen reviewed Oct 25, 2021

View reviewed changes

htwangtw and others added 2 commits October 25, 2021 11:18

Apply suggestions from code review

e9a467b

Co-authored-by: Gensollen <nicolas.gensollen@gmail.com> Co-authored-by: bthirion <bertrand.thirion@inria.fr>

[circle full] Fix the table formatting

091b1c0

bthirion approved these changes Oct 25, 2021

View reviewed changes

NicolasGensollen reviewed Oct 26, 2021

View reviewed changes

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

htwangtw added 3 commits October 26, 2021 10:53

DOCS fix sections in examples

f44d6c2

Merge branch 'main' into fmriprep_confounds_strategy

6fb17de

[circle full] Trigger full build

722d2f1

NicolasGensollen approved these changes Oct 27, 2021

View reviewed changes

NicolasGensollen merged commit d16198d into nilearn:main Oct 29, 2021

NicolasGensollen mentioned this pull request Dec 7, 2021

[ENH] New maskers module #3065

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NF] get fmriprep confounds with fixed strategy #3016

[NF] get fmriprep confounds with fixed strategy #3016

htwangtw commented Oct 12, 2021 •

edited

github-actions bot commented Oct 12, 2021 •

edited by NicolasGensollen

codecov bot commented Oct 12, 2021 •

edited

bthirion left a comment

NicolasGensollen left a comment

NicolasGensollen left a comment

NicolasGensollen Oct 15, 2021

htwangtw Oct 20, 2021

NicolasGensollen Oct 15, 2021

htwangtw Oct 15, 2021

bthirion Oct 17, 2021

htwangtw Oct 18, 2021

bthirion Oct 18, 2021

htwangtw Oct 18, 2021

htwangtw commented Oct 20, 2021 •

edited

NicolasGensollen Oct 21, 2021

htwangtw Oct 22, 2021

bthirion left a comment

bthirion Oct 21, 2021

NicolasGensollen left a comment

bthirion left a comment

NicolasGensollen left a comment

NicolasGensollen left a comment

NicolasGensollen commented Oct 28, 2021

emdupre commented Oct 28, 2021

NicolasGensollen commented Oct 29, 2021

[NF] get fmriprep confounds with fixed strategy #3016

[NF] get fmriprep confounds with fixed strategy #3016

Conversation

htwangtw commented Oct 12, 2021 • edited

github-actions bot commented Oct 12, 2021 • edited by NicolasGensollen

codecov bot commented Oct 12, 2021 • edited

Codecov Report

bthirion left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

NicolasGensollen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htwangtw commented Oct 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasGensollen 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 left a comment

Choose a reason for hiding this comment

NicolasGensollen commented Oct 28, 2021

emdupre commented Oct 28, 2021

NicolasGensollen commented Oct 29, 2021

htwangtw commented Oct 12, 2021 •

edited

github-actions bot commented Oct 12, 2021 •

edited by NicolasGensollen

codecov bot commented Oct 12, 2021 •

edited

htwangtw commented Oct 20, 2021 •

edited