[ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126

ymzayek · 2023-11-30T15:55:04Z

Part of [ENH] Next steps for development on experimental surface module #4027

TODO:

examples to adapt:

examples/04_glm_first_level/plot_localizer_surface_analysis.py
examples/07_advanced/plot_surface_bids_analysis.py
- before: https://nilearn.github.io/dev/auto_examples/07_advanced/plot_surface_bids_analysis.html#group-study
- after: https://output.circle-artifacts.com/output/job/628c6b17-5666-4927-9fbd-4a4e5d838c7e/artifacts/0/dev/auto_examples/08_experimental/plot_new_surface_bids_analysis.html#group-study

github-actions · 2023-11-30T15:55:21Z

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

ymzayek · 2023-11-30T15:56:21Z

@bthirion I started this to work on adapting the glm module. Still very WIP but we can see better what needs to be done. Let me know what you think

codecov · 2023-11-30T16:19:01Z

Codecov Report

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

Comparison is base (2a11133) 92.06% compared to head (a0138f0) 92.06%.

Files	Patch %	Lines
nilearn/glm/first_level/first_level.py	95.12%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4126   +/-   ##
=======================================
  Coverage   92.06%   92.06%           
=======================================
  Files         144      144           
  Lines       16419    16441   +22     
  Branches     3434     3443    +9     
=======================================
+ Hits        15116    15137   +21     
  Misses        761      761           
- Partials      542      543    +1

Flag	Coverage Δ
macos-latest_3.10_test_plotting	`91.86% <95.74%> (+<0.01%)`	⬆️
macos-latest_3.11_test_plotting	`91.86% <95.74%> (+<0.01%)`	⬆️
macos-latest_3.12_test_plotting	`91.86% <95.74%> (+<0.01%)`	⬆️
macos-latest_3.8_test_plotting	`91.82% <95.74%> (+<0.01%)`	⬆️
macos-latest_3.9_test_plotting	`91.82% <95.74%> (+<0.01%)`	⬆️
ubuntu-latest_3.10_test_plotting	`91.86% <95.74%> (+<0.01%)`	⬆️
ubuntu-latest_3.11_test_plotting	`91.86% <95.74%> (+<0.01%)`	⬆️
ubuntu-latest_3.12_test_plotting	`91.86% <95.74%> (+<0.01%)`	⬆️
ubuntu-latest_3.12_test_pre	`91.86% <95.74%> (+<0.01%)`	⬆️
ubuntu-latest_3.8_test_min	`68.96% <89.36%> (+0.01%)`	⬆️
ubuntu-latest_3.8_test_plot_min	`91.57% <95.74%> (+<0.01%)`	⬆️
ubuntu-latest_3.8_test_plotting	`91.82% <95.74%> (+<0.01%)`	⬆️
ubuntu-latest_3.9_test_plotting	`91.82% <95.74%> (+<0.01%)`	⬆️
windows-latest_3.10_test_plotting	`91.83% <95.74%> (+<0.01%)`	⬆️
windows-latest_3.11_test_plotting	`91.83% <95.74%> (+<0.01%)`	⬆️
windows-latest_3.12_test_plotting	`91.83% <95.74%> (+<0.01%)`	⬆️
windows-latest_3.8_test_plotting	`91.79% <95.74%> (+<0.01%)`	⬆️
windows-latest_3.9_test_plotting	`91.80% <95.74%> (+<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.

bthirion · 2023-12-07T20:44:12Z

We're getting close ! It mostly needs a few more tests.

ymzayek · 2023-12-08T13:05:46Z

Regarding L628 if self.mask_img.mask_img_ is None and self.masker_ is None: and what follows it, it seems to me that it is never the case that self.mask_img.mask_img_ is None following the else: condition and checking that the masker has been fitted. It will only be met if the user explicitly sets masker.mask_img_ = None after fitting and this part of the code is tested exactly like that here:

nilearn/nilearn/glm/tests/test_first_level.py

Lines 62 to 72 in 0a08873

    
           # Give a fitted NiftiMasker with a None mask_img_ attribute 
        
           # and check that the masker parameters are overridden by the 
        
           # FirstLevelModel parameters 
        
           masker.fit() 
        
           masker.mask_img_ = None 
        
           with pytest.warns( 
        
               UserWarning, match="Parameter memory of the masker overridden" 
        
           ): 
        
               FirstLevelModel(mask_img=masker).fit( 
        
                   fmri_data[0], design_matrices=design_matrices[0] 
        
               )

I find this a bit odd but maybe there is a reason?

EDIT: The issue below was handled in #4151

Another issue I saw while examining this part of the code that may be worth a separate issue is that the error message when both mask_img and img are None when calling fit on a NiftMasker is *** TypeError: Data given cannot be loaded because it is not compatible with nibabel format: None. This is not clear and I think we should rather mimic what was implemented in SurfaceMasker:

nilearn/nilearn/experimental/surface/_maskers.py

Lines 79 to 82 in 0a08873

    
           raise ValueError( 
        
               "Please provide either a mask_img when initializing " 
        
               "the masker or an img when calling fit()." 
        
           )

bthirion

Thx, this is super important !
Do you plan to add the example in a forthcoming PR ?

nilearn/glm/first_level/first_level.py

nilearn/glm/tests/test_first_level.py

Remi-Gau · 2023-12-11T07:25:46Z

Was going through our backlog of issues and wondering if this PR resolves:

Also wondering more generally if we feel OK closing as "solved" something that is not yet considered "stable" (depends on experimental features).

ymzayek · 2023-12-11T14:03:27Z

@Remi-Gau seems to me to address those. We can list them in the main issue to keep track without closing them for the moment

Remi-Gau · 2023-12-13T21:09:08Z

Do you plan to add the example in a forthcoming PR ?

Going through the PR. What I am seeing makes sense to me. Thanks a bunch @ymzayek

Was trying to think of a follow up example for this.

We mentioned this example during the last hackathon: https://nilearn.github.io/stable/auto_examples/04_glm_first_level/plot_localizer_surface_analysis.html

However this requires to project the fMRI image to the surface which is something we do not yet support with this new Surface object.

Do we have a dataset available with first level surface data?
If not, it may be possible to grab one of the fmriprep preprocessed dataset from openneuro that should have this type of data.

ymzayek · 2024-01-16T09:58:46Z

@bthirion I refactored it a bit by taking the mask section out of the fit into its own ~~_apply_mask~~ _prepare_mask function that is called by fit. But basically it is exactly the same. Maybe this will help make #4112 more straightforward to implement

bthirion · 2024-01-16T21:58:18Z

@bthirion I refactored it a bit by taking the mask section out of the fit into its own ~~_apply_mask~~ _prepare_mask function that is called by fit. But basically it is exactly the same. Maybe this will help make #4112 more straightforward to implement

LGTM, thx.

examples/04_glm_first_level/plot_localizer_surface_analysis.py

Remi-Gau

Looks good to me too. Thanks @ymzayek

bthirion

LGTM, we just need to handle the end of the experimental module.

examples/04_glm_first_level/plot_localizer_surface_analysis.py

nilearn/glm/first_level/first_level.py

ymzayek · 2024-01-18T12:51:00Z

the ideal would be to move everything out of experimental now, but I'm afraid we're too short for the upcoming release ?

@bthirion this is what I'm doing in #4229 so that needs to be merged before this one and then this needs to be rebased and the imports changed. This will be quite easy to do in this PR so the focus should be on finishing 4229. This goes also for the decoder PR (will need to be rebased on 4229 and imports fixed). I used the labels blocker and blocked on these related PRs to try to clarify this dependeny.

Remi-Gau · 2024-01-26T08:21:31Z

examples/08_experimental/plot_localizer_surface_analysis.py

+# We can now simply run a GLM by directly passing
+# our :class:`nilearn.experimental.surface.SurfaceImage` instance
+# as input to FirstLevelModel.fit
+#
+# Here we use an :term:`HRF` model
+# containing the Glover model and its time derivative
+# The drift model is implicitly a cosine basis with a period cutoff at 128s.
+from nilearn.glm.first_level import FirstLevelModel
+
+glm = FirstLevelModel(
+    t_r,
+    slice_time_ref=slice_time_ref,
+    hrf_model="glover + derivative",
+).fit(image, events)


this change compare to the original example

Remi-Gau · 2024-01-26T08:21:46Z

examples/08_experimental/plot_localizer_surface_analysis.py

+# We use the new :class:`nilearn.experimental.surface.SurfaceImage`
+# to create an surface object instance
+# that contains both the mesh
+# (here we use the one from the fsaverage5 templates)
+# and the BOLD data that we project on the surface.
+from nilearn import surface
+from nilearn.experimental.surface import SurfaceImage, load_fsaverage
+
+fsaverage5 = load_fsaverage()
+texture_left = surface.vol_to_surf(
+    fmri_img, fsaverage5["pial"]["left_hemisphere"]
+)
+texture_right = surface.vol_to_surf(
+    fmri_img, fsaverage5["pial"]["right_hemisphere"]
+)
+image = SurfaceImage(
+    mesh={
+        "lh": fsaverage5["pial"]["left_hemisphere"],
+        "rh": fsaverage5["pial"]["right_hemisphere"],
+    },
+    data={
+        "lh": texture_left.T,
+        "rh": texture_right.T,
+    },
+)


this change compare to the original example

Remi-Gau · 2024-01-26T10:22:22Z

examples/08_experimental/plot_new_surface_bids_analysis.py

+# %%
+# Group study
+# -----------
+#
+# Prepare figure for concurrent plot of individual maps
+# compute population-level maps for left and right hemisphere
+# We directly do that on the value arrays.
+import numpy as np
+from scipy.stats import norm, ttest_1samp
+
+_, pval_left = ttest_1samp(np.array(z_scores_left), 0)
+_, pval_right = ttest_1samp(np.array(z_scores_right), 0)
+
+# %%
+# What we have so far are p-values: we convert them to z-values for plotting.
+z_val_left = norm.isf(pval_left)
+z_val_right = norm.isf(pval_right)


Would it make sense to also have second level support for surface images?

Remi-Gau · 2024-01-26T10:40:59Z

examples/08_experimental/plot_new_surface_bids_analysis.py

+for first_level_glm, fmri_img, confound, events in zip(
+    models, run_imgs, confounds, events
+):
+    texture_left = surface.vol_to_surf(
+        fmri_img[0], fsaverage5["pial"]["left_hemisphere"]
+    )
+    texture_right = surface.vol_to_surf(
+        fmri_img[0], fsaverage5["pial"]["right_hemisphere"]
+    )
+    image = SurfaceImage(
+        mesh={
+            "lh": fsaverage5["pial"]["left_hemisphere"],
+            "rh": fsaverage5["pial"]["right_hemisphere"],
+        },
+        data={
+            "lh": texture_left.T,
+            "rh": texture_right.T,
+        },
+    )


given that fmriprep datasets can contain surface data, it would make sense for first_level_from_bids to return the path to the surface images if the space label corresponds to some fsaverage.

Remi-Gau · 2024-01-26T10:52:18Z

probably for a follow up PR

currently for fitting GLM:

in volumes users can pass path to nifti files or nifti objects
for surfaces users must pass a surface object

I think we should have "parity" for those 2 and users should be able to also just pass path to surfaces to fir their GLM.

Remi-Gau · 2024-01-26T12:01:49Z

examples/08_experimental/plot_new_surface_bids_analysis.py

before: https://nilearn.github.io/dev/auto_examples/07_advanced/plot_surface_bids_analysis.html#group-study

after: https://output.circle-artifacts.com/output/job/628c6b17-5666-4927-9fbd-4a4e5d838c7e/artifacts/0/dev/auto_examples/08_experimental/plot_new_surface_bids_analysis.html#group-study

before after

There are small differences but not sure what they are due to.

probably better now since I included the confounds

more changes required

ymzayek added 3 commits November 30, 2023 16:14

Add masker structure

1b785fa

Define surfaceimage

5459d6b

Update

006e258

ymzayek added this to the release 0.11.0 milestone Nov 30, 2023

ymzayek added 7 commits December 6, 2023 15:33

Update docstring

c4ddf21

Skip niimg checks if surface

0da09ae

Fix code breaking tests

b5a4de7

Merge branch 'main' into flm_surfaceimage

5a82233

Add some tests

4a1c25a

Add more tests

415fa4f

Refactor tests

3928ece

Replace parameters for fitted surface masker

a51abfc

ymzayek added 2 commits December 8, 2023 14:16

Add test

66def1e

Remove breakpoint

93230df

ymzayek changed the title ~~[WIP][ENH] Adapt FirstLevelModel to work with surface data objects from new API~~ [ENH] Adapt FirstLevelModel to work with surface data objects from new API Dec 8, 2023

ymzayek marked this pull request as ready for review December 8, 2023 13:57

ymzayek requested review from Remi-Gau and bthirion December 8, 2023 13:57

Adapt compute contrast

8dbdd1d

bthirion reviewed Dec 10, 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

nilearn/glm/tests/test_first_level.py Show resolved Hide resolved

Handle one hemisphere

7b5b130

ymzayek mentioned this pull request Dec 12, 2023

[MAINT] Improve error for nifti masker when fit is called without imgs or mask_img #4151

Merged

ymzayek added 2 commits January 16, 2024 09:43

Clarify doctsring

3c7ae7f

Refactor mask application

04a14cc

Method name and docstring

e08831d

Merge branch 'main' into flm_surfaceimage

6126ed8

Remi-Gau reviewed Jan 17, 2024

View reviewed changes

examples/04_glm_first_level/plot_localizer_surface_analysis.py Outdated Show resolved Hide resolved

Remi-Gau previously approved these changes Jan 17, 2024

View reviewed changes

ymzayek mentioned this pull request Jan 17, 2024

[WIP][ENH] Move surface image API from experimental to stable API #4229

Draft

9 tasks

ymzayek added the Blocked label Jan 17, 2024

bthirion approved these changes Jan 17, 2024

View reviewed changes

examples/04_glm_first_level/plot_localizer_surface_analysis.py Outdated Show resolved Hide resolved

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

Remi-Gau added the Surface Related to surface data or surface analysis. label Jan 23, 2024

Remi-Gau mentioned this pull request Jan 24, 2024

[ENH] Next steps for development on experimental surface module #4027

Open

21 tasks

Remi-Gau modified the milestones: release 0.10.3, Release 0.11.0 Jan 24, 2024

extract example as experimental

9f4d8d0

Remi-Gau reviewed Jan 26, 2024

View reviewed changes

Remi-Gau added 4 commits January 26, 2024 10:09

move example

6ba25f8

update bids example

455887c

clean up

85d6e8a

Merge remote-tracking branch 'upstream/main' into pr/ymzayek/4126

a5de9b3

Remi-Gau reviewed Jan 26, 2024

View reviewed changes

adapt fetcher

661843e

Remi-Gau reviewed Jan 26, 2024

View reviewed changes

use confounds

a0138f0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126

[ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126

ymzayek commented Nov 30, 2023 •

edited by Remi-Gau

github-actions bot commented Nov 30, 2023

ymzayek commented Nov 30, 2023

codecov bot commented Nov 30, 2023 •

edited

bthirion commented Dec 7, 2023

ymzayek commented Dec 8, 2023 •

edited

bthirion left a comment

Remi-Gau commented Dec 11, 2023

ymzayek commented Dec 11, 2023

Remi-Gau commented Dec 13, 2023

ymzayek commented Jan 16, 2024 •

edited

bthirion commented Jan 16, 2024

Remi-Gau left a comment

bthirion left a comment

ymzayek commented Jan 18, 2024

Remi-Gau Jan 26, 2024

Remi-Gau Jan 26, 2024

Remi-Gau Jan 26, 2024

Remi-Gau Jan 26, 2024

Remi-Gau Jan 26, 2024

Remi-Gau commented Jan 26, 2024

Remi-Gau Jan 26, 2024 •

edited

Remi-Gau Jan 26, 2024

Remi-Gau Jan 26, 2024

[ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126

Are you sure you want to change the base?

[ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126

Conversation

ymzayek commented Nov 30, 2023 • edited by Remi-Gau

github-actions bot commented Nov 30, 2023

ymzayek commented Nov 30, 2023

codecov bot commented Nov 30, 2023 • edited

Codecov Report

bthirion commented Dec 7, 2023

ymzayek commented Dec 8, 2023 • edited

bthirion left a comment

Choose a reason for hiding this comment

Remi-Gau commented Dec 11, 2023

ymzayek commented Dec 11, 2023

Remi-Gau commented Dec 13, 2023

ymzayek commented Jan 16, 2024 • edited

bthirion commented Jan 16, 2024

Remi-Gau left a comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

ymzayek commented Jan 18, 2024

Remi-Gau Jan 26, 2024

Choose a reason for hiding this comment

Remi-Gau Jan 26, 2024

Choose a reason for hiding this comment

Remi-Gau Jan 26, 2024

Choose a reason for hiding this comment

Remi-Gau Jan 26, 2024

Choose a reason for hiding this comment

Remi-Gau Jan 26, 2024

Choose a reason for hiding this comment

Remi-Gau commented Jan 26, 2024

Remi-Gau Jan 26, 2024 • edited

Choose a reason for hiding this comment

Remi-Gau Jan 26, 2024

Choose a reason for hiding this comment

Remi-Gau Jan 26, 2024

Choose a reason for hiding this comment

ymzayek commented Nov 30, 2023 •

edited by Remi-Gau

codecov bot commented Nov 30, 2023 •

edited

ymzayek commented Dec 8, 2023 •

edited

ymzayek commented Jan 16, 2024 •

edited

Remi-Gau Jan 26, 2024 •

edited