[ENH] integrate load_confounds into first_level_from_bids #4103

Remi-Gau · 2023-11-09T11:43:37Z

Closes [ENH] integrate load_confounds into first_level_from_bids #4090

Changes proposed in this pull request:

use load_confounds to load specific subset of confounds if extra confounds_* arguments are passed
update fake bids data generation so they contain more realistic confounds

TODO

update changelog
investigate conflicts between confounds strategy and GLM detrenting
add in example?

github-actions · 2023-11-09T11:43:48Z

👋 @Remi-Gau 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 (see our documentation on PR structure)
Code is PEP8-compliant (see our documentation on coding style)
Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

There is at least one unit test per new function / class (see our documentation on testing)
The new feature is demoed in at least one relevant example.

For bug fixes:

There is at least one test that would fail under the original bug conditions.

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

Remi-Gau · 2023-11-09T11:44:25Z

nilearn/_utils/bids.py

had to move the create_bids_filename in a different module to help avoid circular imports

Remi-Gau · 2023-11-09T11:55:19Z

nilearn/interfaces/fmriprep/tests/_testing.py

some of these changes in here are black related

may conflict with #3285

Remi-Gau · 2023-11-09T11:56:23Z

nilearn/glm/tests/test_first_level.py

+    models, m_imgs, m_events, m_confounds = first_level_from_bids(
+        dataset_path=bids_path,
+        task_label="main",
+        space_label="MNI",
+        img_filters=[("desc", "preproc")],
+        slice_time_ref=None,
+        confounds_strategy=("motion", "wm_csf", "scrub"),
+        confounds_motion="full",
+        confounds_wm_csf="basic",
+        confounds_scrub=1,
+        confounds_fd_threshold=0.2,
+        confounds_std_dvars_threshold=3,
+    )


this what the "API" could look like to select some confounds: does it look sensible?

@htwangtw @ymzayek @bthirion
keeping this as a draft so you can discuss API, implementation...

Will update the doc once the dust settles.

I guess this could be made lighter if we expect to have predefined configurations for confounds, but I don't think that we're at that point.
At least I find the current API quite explicit.

Actually, we might get rid of confounds_strategy, because the next three arguments are redundant with the provided list ?

Note this test just show a subset of the possible arguments that can be passed to load_confounds

I guess this could be made lighter if we expect to have predefined configurations for confounds, but I don't think that we're at that point.

Actually, we might get rid of confounds_strategy, because the next three arguments are redundant with the provided list ?

Actually I am just reusing the API from load_confounds, so it can almost be passed as is and we can let load_confounds do the argument validation

https://nilearn.github.io/dev/modules/generated/nilearn.interfaces.fmriprep.load_confounds.html

In short strategy defines what type of confounds to include and all the other parameters give more details on how to include them.

will update the doc string to try to explain this so we can see if it makes sense

Remi-Gau · 2023-11-09T12:00:58Z

one thing we may want to check and send warning about the compatibility between some confound strategies and first level arguments: especially regarding high pass filters

Remi-Gau · 2023-11-09T12:39:55Z

nilearn/_utils/data_gen.py

+        confounds, metadata = get_legal_confound()
+        confounds.to_csv(
+            confounds_path, sep="\t", index=None, encoding="utf-8"
        )
+        with open(confounds_path.with_suffix(".json"), "w") as f:
+            json.dump(metadata, f)


One issue with this approach is that the "legal_confounds" have a set number of time points so when creating a fake bids dataset, we end up with images that have a number of time points that does not match the number of time points in the confounds.

This does not affect any tests AFIACT but this may lead to confusing errors when testing down the line.

You mean, because of scrubbing ? Sorry if I miss something obvious.

nope

let me try to rephrase

the way to generate "fake confounds" for the fake fmriprep datasets we use for testing would only create 6 confounds for the realignment parameters filled with random data and for a specified number of time points

to allow testing the load_confounds we need more realistic confounds with more columns with names that match what's in an actual fmriprep dataset

to do this I reuse the strategy used to test the load_confounds functions: use an actual confound file from an fmriprep dataset and copy its content every time it is needed in the fake fmriprep dataset

but this "template" confound file has only a limited number of time points

so we end up with fake fmriprep datasets that have nifti images with 100 volumes but with confounds with only 30 time points

possible solutions:

easy: set the number of volumes to match the number of time points in the confounds

hard(er): adapt the content of the confounds to the number of time points

hope this is clearer

for now I will go for the easy solution but we may have to implement the harder option in the future if we want to test more "exotic" stuff

Let's go for the easy one. The number of volumes should be a parameters of the data simulation function anyhow ?

for now it is not: it is hard coded. I would keep it that way for now until we need more flexibility during testing.

but I will change the place where it is hard coded so it is easier to adapt in the future. will also add a comment to explain why this value was chosen.

Remi-Gau · 2023-11-09T13:39:14Z

nilearn/_utils/data_gen.py

+        with open(confounds_path.with_suffix(".json"), "w") as f:
+            json.dump(metadata, f)


minor change: using legal_confounds allows to add metada files for the confounds in the fake bids derivatives. some tests had to be changed to account for this.

codecov · 2023-11-09T14:18:07Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (a1810e3) 91.85% compared to head (d3588e1) 91.86%.

Files	Patch %	Lines
nilearn/_utils/bids.py	86.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4103      +/-   ##
==========================================
+ Coverage   91.85%   91.86%   +0.01%     
==========================================
  Files         145      146       +1     
  Lines       16360    16384      +24     
  Branches     3424     3432       +8     
==========================================
+ Hits        15027    15051      +24     
+ Misses        792      788       -4     
- Partials      541      545       +4

Flag	Coverage Δ
macos-latest_3.10_test_plotting	`91.72% <96.49%> (+0.01%)`	⬆️
macos-latest_3.11_test_plotting	`?`
macos-latest_3.12_test_plotting	`91.72% <96.49%> (+0.01%)`	⬆️
macos-latest_3.8_test_plotting	`91.68% <96.49%> (+0.01%)`	⬆️
macos-latest_3.9_test_plotting	`91.69% <96.49%> (+0.01%)`	⬆️
ubuntu-latest_3.10_test_plotting	`?`
ubuntu-latest_3.11_test_plotting	`?`
ubuntu-latest_3.12_test_plotting	`91.72% <96.49%> (+0.01%)`	⬆️
ubuntu-latest_3.12_test_pre	`91.72% <96.49%> (+0.01%)`	⬆️
ubuntu-latest_3.8_test_min	`68.95% <96.49%> (?)`
ubuntu-latest_3.8_test_plot_min	`?`
ubuntu-latest_3.8_test_plotting	`?`
ubuntu-latest_3.9_test_plotting	`?`
windows-latest_3.8_test_plotting	`91.66% <96.49%> (+0.01%)`	⬆️
windows-latest_3.9_test_plotting	`91.66% <96.49%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Remi-Gau · 2023-11-09T15:12:04Z

Thought: should this functionality be demoed in the examples?

Remi-Gau · 2023-11-10T17:18:55Z

nilearn/glm/first_level/first_level.py

+    kwargs: :obj:`dict`
+
+        .. added:: 0.11.0
+
+    Keyword arguments to be passed to functions called within this function.
+
+    Kwargs prefixed with ``confound_``
+    will be passed to :func:`~nilearn.interfaces.fmriprep.load_confounds`.
+    This allows to ``first_level_from_bids`` to return
+    a specific set of confounds by relying confound loading strategies
+    defined in :func:`~nilearn.interfaces.fmriprep.load_confounds`.
+    If no kwargs are passed, ``first_level_from_bids`` will return
+    all the confounds available in the confounds TSV files.


@bthirion let me know if this helps clarify how to use this.

I prefer to add examples here and refer users to the doc of load_confounds for the details to avoid doc duplications.

bthirion · 2023-11-13T22:20:17Z

Is this ready for review ?

Remi-Gau · 2023-11-14T09:34:55Z

I would say yes, though I need to better check how to handle the second TODO mentioned in the top post of the PR:

investigate conflicts between confounds strategy and GLM detrenting

bthirion

LGTM overall !

bthirion · 2023-11-14T21:38:43Z

nilearn/glm/first_level/first_level.py

+
+    .. code-block:: python
+
+        models, m_imgs, m_events, m_confounds = first_level_from_bids(


Suggested change

models, m_imgs, m_events, m_confounds = first_level_from_bids(

models, imgs, events, confounds = first_level_from_bids(

``` (not sure what the `m_` means)

That was a copy-pasta from the code we have in the tests: probably could change it there too.

FYI those were shorter form of the name of the return argument:

models_run_imgs

models_events

models_confounds

So maybe even if this makes the doc more verbose, I should use their full name to be internally consistent in the doc string ?

I would prefer models, imgs, events, confounds = first_level_from_bids

@bthirion I renamed those variable in the doc strings.

they also appear like this in a few tests, I could do a bit of renaming there too just for internal consistency.

nilearn/glm/first_level/first_level.py

ymzayek

Looking good!

nilearn/glm/first_level/first_level.py

ymzayek · 2023-11-15T16:56:32Z

I'm not against demoing in an example. I would always just consider the amount of additional build time, whether or not it is already well documented (which the added docstring does very well), and if parts of an example can be replaced or minimally tweaked to essentially improve them using this new functionality.

Remi-Gau · 2023-11-16T09:26:31Z

I'm not against demoing in an example. I would always just consider the amount of additional build time, whether or not it is already well documented (which the added docstring does very well), and if parts of an example can be replaced or minimally tweaked to essentially improve them using this new functionality.

I will do a draft in a separate PR but I was considering a minimal tweak to an already existing example.

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

Remi-Gau · 2023-12-07T17:08:05Z

"Conflict" to resolve:

load_confounds

load_confounds has the possibility to add a high pass filter to the confounds

- "high_pass" adds discrete cosines transformation basis regressors to handle low-frequency signal drifts.

and when the compcor strategy is requested "high_pass" must be as well

- "compcor" confounds derived from CompCor :footcite:`Behzadi2007`.
  When using this noise component, "high_pass" must also be applied.
  Associated parameter: `compcor`, `n_compcor`

GLM first level

drift_model can be cosine or polynomial or none

    drift_model : string, default='cosine'
        This parameter specifies the desired drift model for the design
        matrices. It can be 'polynomial', 'cosine' or None.

    high_pass : float, default=0.01
        This parameter specifies the cut frequency of the high-pass filter in
        Hz for the design matrices. Used only if drift_model is 'cosine'.

    drift_order : int, default=1
        This parameter specifies the order of the drift model (in case it is
        polynomial) for the design matrices.

There are some combinations of the above that could lead to "strange" design matrices:

design matrices with high pass filter defined twice (with possibly different types of high pass filters)

To keep things simple I would say that we let the GLM machinery handle the high pass filtering setting and we ignore in the load_confounds anything that has to do with high pass filtering.

This means that we should also ignore (for now) the compcor strategy.

Remi-Gau · 2023-12-07T17:10:06Z

@bthirion before I start on this, can you tell me if the brief description above makes roughly sense as to what the problem is?

bthirion · 2023-12-07T21:02:22Z

You mean, the problem of duplication of high-pass filtering with the use of compcorr ? My view on this is that this is the user responsibility. You can't prevent people from doing wrong things. The point is to help them diagnose it easily using visualization of the design matrix they created. Does that answer your concern ?

Remi-Gau · 2023-12-11T10:36:07Z

You can't prevent people from doing wrong things but you can add more friction to make it harder for them to do so. 😋

I think my views may be a bit more "paternalistic" (more tainted by automated pipelines and avoiding options that would considered wrong in most cases) but you have way more experience than me in the "nilearn philosophy" so I will gladly follow your lead on this.

I will at least add some explicit warnings to tell them they have chosen options that may be redundant or clash with each other.

bthirion · 2023-12-11T11:29:07Z

Agreed. My impression is that when we impose constraints, we may not foresee all practical consequences.
This is why I think it is important to have good examples or event tutorials that promote good patterns.

htwangtw · 2023-12-11T14:52:05Z

Currently we do safegard the compcor and high pass in load_confounds. For real I have already seen people using other approaches for high pass that is not the cosine regressor approach with fMRIPrep compcor despite that is not recommanded on their official documentation. Raising warning might be the way to go

Remi-Gau · 2023-12-13T13:05:48Z

nilearn/glm/tests/test_first_level.py

-    models, m_imgs, m_events, m_confounds = first_level_from_bids(
+    models, models, imgs, events, confounds = first_level_from_bids(


renamed the variables as mentioned in the PR discussion

Remi-Gau · 2023-12-13T13:07:31Z

nilearn/glm/first_level/first_level.py

+    if drift_model is not None and kwargs_load_confounds is not None:
+        if "high_pass" in kwargs_load_confounds.get("strategy"):
+            if drift_model == "cosine":
+                verb = "duplicate"
+            if drift_model == "polynomial":
+                verb = "conflict with"
+
+            warn(
+                f"""Confounds will contain a high pass filter,
+ that may {verb} the {drift_model} one used in the model.
+ Remember to visualize your design matrix before fitting your model
+ to check that your model is not overspecified.""",


Not sure about the phrasing of the warning.

looks reasonable to me

To me toot.

bthirion

LGTM, thx.

bthirion · 2023-12-14T01:39:12Z

nilearn/glm/first_level/first_level.py

+    if drift_model is not None and kwargs_load_confounds is not None:
+        if "high_pass" in kwargs_load_confounds.get("strategy"):
+            if drift_model == "cosine":
+                verb = "duplicate"
+            if drift_model == "polynomial":
+                verb = "conflict with"
+
+            warn(
+                f"""Confounds will contain a high pass filter,
+ that may {verb} the {drift_model} one used in the model.
+ Remember to visualize your design matrix before fitting your model
+ to check that your model is not overspecified.""",


To me toot.

Remi-Gau · 2023-12-15T15:45:00Z

If I don't hear back from anyone I will merge this on Monday.

htwangtw

LGTM thanks for implementing this!

Remi-Gau added 2 commits November 9, 2023 11:34

start integrating loading of confounds

fb14470

load confounds for glm

84914ea

Remi-Gau commented Nov 9, 2023

View reviewed changes

nilearn/_utils/bids.py Outdated

Copy link

Collaborator Author

Remi-Gau Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to move the create_bids_filename in a different module to help avoid circular imports

updating black config

a3ff639

Remi-Gau commented Nov 9, 2023

View reviewed changes

Remi-Gau marked this pull request as draft November 9, 2023 11:56

Remi-Gau commented Nov 9, 2023

View reviewed changes

fix tests

ecce8e2

Remi-Gau commented Nov 9, 2023

View reviewed changes

Remi-Gau added 5 commits November 10, 2023 11:34

match number of time points to that in confounds file

659794a

Merge remote-tracking branch 'upstream/main' into bids_glm_confounds

815bdc0

fix merge conflicts

a21a4c4

update changelog

eb16889

update doc

56e580d

Remi-Gau commented Nov 10, 2023

View reviewed changes

fix typos

4d1cc24

Remi-Gau marked this pull request as ready for review November 14, 2023 09:33

bthirion reviewed Nov 14, 2023

View reviewed changes

ymzayek reviewed Nov 15, 2023

View reviewed changes

nilearn/glm/first_level/first_level.py Outdated Show resolved Hide resolved

nilearn/glm/first_level/first_level.py Outdated Show resolved Hide resolved

nilearn/glm/first_level/first_level.py Outdated Show resolved Hide resolved

Remi-Gau and others added 2 commits November 16, 2023 10:27

Apply suggestions from code review

23444e5

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

rename output var in doc string

a79055e

Merge remote-tracking branch 'upstream/main' into bids_glm_confounds

103e326

nilearn deleted a comment from D3njo Dec 12, 2023

Remi-Gau added 3 commits December 13, 2023 13:47

add warning

c4089c8

improve warning

9f77bcc

rename variables

b457a11

Remi-Gau commented Dec 13, 2023

View reviewed changes

Remi-Gau added 2 commits December 13, 2023 14:19

check names of confounds

c043f78

fix hasty ccopy paste

a62c27c

bthirion approved these changes Dec 14, 2023

View reviewed changes

Remi-Gau added 3 commits December 14, 2023 11:53

Merge branch 'main' into bids_glm_confounds

93ab4f7

lint

a8a1eb1

Merge remote-tracking branch 'upstream/main' into bids_glm_confounds

d3588e1

htwangtw approved these changes Dec 15, 2023

View reviewed changes

Remi-Gau merged commit dfe2d54 into nilearn:main Dec 18, 2023
32 checks passed

Remi-Gau deleted the bids_glm_confounds branch December 18, 2023 07:12

Remi-Gau mentioned this pull request Dec 23, 2023

[ENH] update example to show how to use load_confounds with first_level_from_bids #4178

Open

		with open(confounds_path.with_suffix(".json"), "w") as f:
		json.dump(metadata, f)


		.. code-block:: python

		models, m_imgs, m_events, m_confounds = first_level_from_bids(

	models, m_imgs, m_events, m_confounds = first_level_from_bids(
	models, imgs, events, confounds = first_level_from_bids(
	``` (not sure what the `m_` means)

		models, m_imgs, m_events, m_confounds = first_level_from_bids(
		models, models, imgs, events, confounds = first_level_from_bids(

[ENH] integrate load_confounds into first_level_from_bids #4103

[ENH] integrate load_confounds into first_level_from_bids #4103

Conversation

Remi-Gau commented Nov 9, 2023 • edited

TODO

github-actions bot commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau Nov 9, 2023 • edited

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

Remi-Gau commented Nov 9, 2023

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

codecov bot commented Nov 9, 2023 • edited

Codecov Report

Remi-Gau commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bthirion commented Nov 13, 2023

Remi-Gau commented Nov 14, 2023

bthirion 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

ymzayek left a comment

Choose a reason for hiding this comment

ymzayek commented Nov 15, 2023

Remi-Gau commented Nov 16, 2023

Remi-Gau commented Dec 7, 2023

load_confounds

GLM first level

Remi-Gau commented Dec 7, 2023

bthirion commented Dec 7, 2023 • edited

Remi-Gau commented Dec 11, 2023

bthirion commented Dec 11, 2023

htwangtw commented Dec 11, 2023

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

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Dec 15, 2023

htwangtw left a comment

Choose a reason for hiding this comment

Remi-Gau commented Nov 9, 2023 •

edited

Remi-Gau Nov 9, 2023 •

edited

codecov bot commented Nov 9, 2023 •

edited

bthirion commented Dec 7, 2023 •

edited