[ENH] add plotting functions to the experimental surface module #4235

jeromedockes · 2024-01-23T18:02:59Z

added a placeholder basic plotting function

closes [DOC] experimental-plot-surface-image-and-maskers example does not render well #4098

TODO

~~change PolyMesh, PolyData to constrain keys to be "left" and "right"~~ (required for plotting when we want medial or lateral view)
split experimental package into: maskers, plotting, datasets to make them easier to later integrate into the rest of the code base
add functions to return fsaverage data (sulcal, curvature...) as SurfaceImage instance with specific mesh (pial, flat...)
bg_map of the new plotting functions can be a SurfaceImage instance
add method to SurfaceImage to check the presence of a given hemisphere in data and mesh
more plotting functions

Examples to adapt:

examples/01_plotting/plot_3d_map_to_surface_projection.py
examples/01_plotting/plot_surf_atlas.py
examples/01_plotting/plot_surf_stat_map.py

use "left" and "right" instead of "left_hemisphere" for surface parts

github-actions · 2024-01-23T18:03:14Z

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

codecov · 2024-01-23T18:25:27Z

Codecov Report

Attention: Patch coverage is 80.74866% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 92.03%. Comparing base (abb80ff) to head (a2f0a55).
Report is 49 commits behind head on main.

Files	Patch %	Lines
nilearn/experimental/plotting/_surface_plotting.py	57.57%	21 Missing and 7 partials ⚠️
nilearn/experimental/surface/_datasets.py	84.00%	2 Missing and 2 partials ⚠️
nilearn/experimental/surface/_surface_image.py	20.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4235      +/-   ##
==========================================
+ Coverage   91.85%   92.03%   +0.17%     
==========================================
  Files         144      146       +2     
  Lines       16419    16534     +115     
  Branches     3434     3463      +29     
==========================================
+ Hits        15082    15217     +135     
+ Misses        792      765      -27     
- Partials      545      552       +7

Flag	Coverage Δ
macos-latest_3.10_test_plotting	`91.82% <80.74%> (?)`
macos-latest_3.11_test_plotting	`91.82% <80.74%> (-0.03%)`	⬇️
macos-latest_3.8_test_plotting	`91.78% <80.74%> (?)`
ubuntu-latest_3.10_test_plotting	`91.82% <80.74%> (-0.03%)`	⬇️
ubuntu-latest_3.11_test_plotting	`91.82% <80.74%> (?)`
ubuntu-latest_3.12_test_plotting	`91.82% <80.74%> (?)`
ubuntu-latest_3.12_test_pre	`91.82% <80.74%> (?)`
ubuntu-latest_3.8_test_min	`68.76% <59.32%> (?)`
ubuntu-latest_3.8_test_plot_min	`91.54% <80.74%> (?)`
ubuntu-latest_3.8_test_plotting	`91.78% <80.74%> (?)`
ubuntu-latest_3.9_test_plotting	`91.79% <80.74%> (?)`
windows-latest_3.10_test_plotting	`91.81% <80.74%> (?)`
windows-latest_3.11_test_plotting	`91.81% <80.74%> (?)`
windows-latest_3.12_test_plotting	`91.81% <80.74%> (?)`
windows-latest_3.8_test_plotting	`91.77% <80.74%> (?)`
windows-latest_3.9_test_plotting	`91.78% <80.74%> (?)`

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.

…` attribute This allows enforcing the fact that at least one key is present and keys must be in ["left", "right"], and exposing the `shape` and `n_vertices` attributes

bthirion

Thx for kicking that off !

Remi-Gau · 2024-01-24T07:40:05Z

Note to ourselves: most likely there will be some merge conflicts with #4229

jeromedockes · 2024-01-24T08:23:20Z

Note to ourselves: most likely there will be some merge conflicts with #4229

yes 😰

Remi-Gau · 2024-01-24T09:30:27Z

@jeromedockes
to maybe minimize conflicts, maybe check #4229 and let me know if some bits should be adapted to make it easier to merge this after.

jeromedockes · 2024-01-24T12:29:02Z

@jeromedockes to maybe minimize conflicts, maybe check #4229 and let me know if some bits should be adapted to make it easier to merge this after.

sure, sorry I haven't had time to follow that as closely as I would have wanted. Are you and Yasmin going to the office hours today? Maybe we can use it to do a quick catch-up on the remaining outstanding points

Remi-Gau · 2024-01-24T12:57:49Z

I will be there so we can plan the next steps then

Remi-Gau · 2024-01-26T08:40:26Z

@jeromedockes

added the examples to adapt in the top message of the PR.

I am OK to tackle those in a separate PR if you prefer. This is just to let you know.

Remi-Gau · 2024-01-29T08:12:53Z

@jeromedockes
want me to take over this one?

jeromedockes · 2024-02-02T08:28:31Z

@jeromedockes want me to take over this one?

Hi Remi sorry for the delay. Please feel free to do so, no problem but actually I think it might be easier to start a new one as most of the diff in this one is to add the `.parts` attribute to polymesh and poly data, which in the end we decided against during the office hours

Remi-Gau · 2024-02-05T09:28:30Z

@jeromedockes

Will try to adapt this one first. It cannot take THAT long to revert. #FamousLastWords

Remi-Gau · 2024-02-12T09:17:15Z

PR is getting pretty long because of:

making copies of the examples to adapt into experimental examples to test the new plotting functions.
splitting the experimental module further into datasets, maskers...

Probably worth stopping here for now and make further changes in separate PR.

I can also split the examples in different PRs if easier for review.

Remi-Gau · 2024-02-12T09:18:00Z

@jeromedockes if you have time to cross check 😉

Remi-Gau · 2024-02-12T09:33:08Z

nilearn/experimental/plotting/_surface_plotting.py

+    surf_mesh: (
+        str | list[numpy.array, numpy.array] | Mesh | PolyMesh | None
+    ) = None,


given that currently surface_mesh can be a str pointing to a file containing a mesh, I think it may be appropriate to also be able to pass a SurfaceImage to surf_mesh

like is done for bg_map

Note sure I follow you here.

Not 100% sure about this, so to be discussed but it seems that it would convenient if users could directly pass a SurfaceImage instance to this parameter and let nilearn get the Polymesh from the instance rather than forcing users to pass just a Mesh / PolyMesh.

I may be overthinking this but if users can just deal with fewer types of objects (SurfaceImage).

it would be weird to pass a different image than the one we are plotting just to grab the mesh from it. either the mesh is None in which case we use the mesh that comes with surf_map, or we want to provide a different mesh (eg an inflated surface) in which case we pass surf_mesh, which should be a mesh. if it happens to be stored as part of another image (which is not very likely if it is an inflated or flat surface used just for plotting) the user can always do img.mesh to access it

okidoki
not a hill I wanna die on and if users request it we can think about changing that later

but then do you think that it makes sense to be able to pass a SurfaceImage for the background map the way I have done?

Remi-Gau · 2024-02-12T09:35:35Z

examples/08_experimental/plot_3d_map_to_surface_projection_experimental.py

+img = SurfaceImage(
+    mesh=fsaverage_meshes["pial"],
+    data={
+        "left": surface.vol_to_surf(
+            stat_img, fsaverage_meshes["pial"]["left"]
+        ),
+        "right": surface.vol_to_surf(
+            stat_img, fsaverage_meshes["pial"]["right"]
+        ),
+    },
+)


TODO in another PR: add method to SurfaceImage to handle vol to surf

surfaceimage is for holding surface data, i don't think it should be responsible for converting volumes

Remi-Gau · 2024-02-12T10:18:27Z

nilearn/experimental/plotting/_surface_plotting.py

+        surf_mesh = surf_map.mesh
+
+    surf_map._check_hemi(hemi)
+    _check_hemi_present(surf_mesh, hemi)


this will fail if surf_mesh is not a PolyMesh
fixing this will probably need better define the PolyMesh class

Can you clarify the discussion here ?

_check_hemi_present is just to make sure that the "PolyMesh" dict has the correct hemisphere

But most of our new code at the moment, does not use the PolyMesh class but simple dictionaries instead, so we cannot do a isinstance(surf_mesh, PolyMesh). So we should probably develop the PolyMesh class, because at the moment it is only PolyMesh = Dict[str, Mesh].

Also at this stage surf_mesh can also be a str, list of two numpy.ndarray, Mesh and none of those cases are handled.

Not I could start making those changes in this PR but I was getting afraid of having an even bigger PR

Remi-Gau · 2024-02-12T11:25:43Z

nilearn/experimental/datasets/_datasets.py

+    return images
+
+
+def fetch_destrieux(


testing this will require improving the mocking of the dataset: would probably do this in another PR

bthirion

Needs more discussion.

bthirion · 2024-02-12T22:12:57Z

nilearn/experimental/datasets/_datasets.py

+        }
+        for mesh_type, mesh_name in renaming.items()
+    }
+    renaming = {"curv": "curvature", "sulc": "sulcal", "thick": "thickness"}


Why are you doing this here ?

bthirion · 2024-02-12T22:23:18Z

nilearn/experimental/plotting/_surface_plotting.py

+    surf_mesh: (
+        str | list[numpy.array, numpy.array] | Mesh | PolyMesh | None
+    ) = None,


Note sure I follow you here.

bthirion · 2024-02-12T22:23:49Z

nilearn/experimental/plotting/_surface_plotting.py

+        surf_mesh = surf_map.mesh
+
+    surf_map._check_hemi(hemi)
+    _check_hemi_present(surf_mesh, hemi)


Can you clarify the discussion here ?

nilearn/experimental/datasets/_datasets.py

jeromedockes

ok I'll try to have a look this week. My first comment is I don't see the need for the change from

module.py

to

module/
   __init__.py (empty besides grabbing names from _module.py)
   _module.py
   (no other submodules)

jeromedockes · 2024-02-13T08:47:24Z

nilearn/experimental/plotting/_surface_plotting.py

+    surf_mesh: (
+        str | list[numpy.array, numpy.array] | Mesh | PolyMesh | None
+    ) = None,


it would be weird to pass a different image than the one we are plotting just to grab the mesh from it. either the mesh is None in which case we use the mesh that comes with surf_map, or we want to provide a different mesh (eg an inflated surface) in which case we pass surf_mesh, which should be a mesh. if it happens to be stored as part of another image (which is not very likely if it is an inflated or flat surface used just for plotting) the user can always do img.mesh to access it

Remi-Gau · 2024-02-13T08:57:49Z

ok I'll try to have a look this week. My first comment is I don't see the need for the change from
module.py
to
module/
   __init__.py (empty besides grabbing names from _module.py)
   _module.py
   (no other submodules)

For this I was mostly thinking of making a bit easier to integrate all this code into the rest of the code base later.

jeromedockes · 2024-02-13T08:58:59Z

but then do you think that it makes sense to be able to pass a SurfaceImage for the background map the way I have done?

yes because the background map is an actual image that has both a mesh and data, right?

Remi-Gau · 2024-02-13T09:03:09Z

but then do you think that it makes sense to be able to pass a SurfaceImage for the background map the way I have done?
yes because the background map is an actual image that has both a mesh and data, right?

yeah that was my reasoning, but your argument of saying:

"well users should just do img.data"

could also sort of apply: so that's why I was feeling a bit awkward to say you can do it for background data but not meshes...

Remi-Gau · 2024-02-14T12:52:41Z

nilearn/datasets/func.py

@@ -2200,13 +2200,12 @@ def fetch_language_localizer_demo_dataset(
        - 'description' : :obj:`str`, dataset description

    Legacy output
-    -------------


removes a warning from the doc build

Remi-Gau · 2024-02-14T13:03:59Z

@jeromedockes
I moved things back into a single subpackage.

Let me know if you think that I should start creating "proper" classes for PolyMesh and PolyData in this PR or not

jeromedockes added 4 commits November 24, 2023 16:38

add experimental plotting module

9c901d4

Merge remote-tracking branch 'upstream/main' into surface-plotting

a3114f9

basic surface plotting function

41ef206

use "left" and "right" instead of "left_hemisphere" for surface parts

Merge remote-tracking branch 'upstream/main' into surface-plotting

9da44f6

jeromedockes marked this pull request as draft January 23, 2024 18:03

add PolyMesh and PolyData classes, data is now stored in their `parts…

0edd273

…` attribute This allows enforcing the fact that at least one key is present and keys must be in ["left", "right"], and exposing the `shape` and `n_vertices` attributes

bthirion reviewed Jan 23, 2024

View reviewed changes

Remi-Gau added this to the release 0.11.0 milestone Jan 24, 2024

Remi-Gau added Surface Related to surface data or surface analysis. Plotting The issue is related to plotting functionalities. labels Jan 24, 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 changed the title ~~[WIP] add some plotting functions to the experimental surface module~~ [WIP][ENH] add some plotting functions to the experimental surface module Jan 24, 2024

Remi-Gau mentioned this pull request Jan 24, 2024

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

Draft

9 tasks

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

Remi-Gau added 4 commits February 7, 2024 18:22

use dict

586538f

restore some files from main

1c1c7ea

revert to dict

f94d125

update tests and example

8affc03

Remi-Gau requested a review from bthirion February 12, 2024 09:17

refactor and update doc$

22d2d43

Remi-Gau reviewed Feb 12, 2024

View reviewed changes

mock nki

c9a1b1e

Remi-Gau reviewed Feb 12, 2024

View reviewed changes

Remi-Gau marked this pull request as ready for review February 12, 2024 12:51

Remi-Gau changed the title ~~[WIP][ENH] add some plotting functions to the experimental surface module~~ [ENH] add some plotting functions to the experimental surface module Feb 12, 2024

bthirion reviewed Feb 12, 2024

View reviewed changes

Remi-Gau reviewed Feb 13, 2024

View reviewed changes

nilearn/experimental/datasets/_datasets.py Outdated Show resolved Hide resolved

Update nilearn/experimental/datasets/_datasets.py

cc852ff

jeromedockes commented Feb 13, 2024

View reviewed changes

Remi-Gau added 2 commits February 14, 2024 13:35

refactor

2dad51b

rm import fixtures

7348e7f

Remi-Gau reviewed Feb 14, 2024

View reviewed changes

fix doc

93d0818

Merge remote-tracking branch 'upstream/main' into pr/jeromedockes/4235

c76b807

Remi-Gau changed the title ~~[ENH] add some plotting functions to the experimental surface module~~ [ENH] add plotting functions to the experimental surface module Feb 28, 2024

Remi-Gau added 3 commits February 28, 2024 09:49

fix import fixtures

6cdb067

misc

ad0a075

improve examples

a2f0a55

Remi-Gau added the Blocker label Mar 8, 2024

Remi-Gau added the Priority: high The task is urgent and needs to be addressed as soon as possible. label Apr 9, 2024

[ENH] add plotting functions to the experimental surface module #4235

Are you sure you want to change the base?

[ENH] add plotting functions to the experimental surface module #4235

Conversation

jeromedockes commented Jan 23, 2024 • edited by Remi-Gau

github-actions bot commented Jan 23, 2024

codecov bot commented Jan 23, 2024 • edited

Codecov Report

bthirion left a comment

Choose a reason for hiding this comment

Remi-Gau commented Jan 24, 2024

jeromedockes commented Jan 24, 2024

Remi-Gau commented Jan 24, 2024

jeromedockes commented Jan 24, 2024 via email

Remi-Gau commented Jan 24, 2024

Remi-Gau commented Jan 26, 2024

Remi-Gau commented Jan 29, 2024

jeromedockes commented Feb 2, 2024 via email

Remi-Gau commented Feb 5, 2024

Remi-Gau commented Feb 12, 2024

Remi-Gau commented Feb 12, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromedockes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Feb 13, 2024

jeromedockes commented Feb 13, 2024 via email

Remi-Gau commented Feb 13, 2024 • edited

Choose a reason for hiding this comment

Remi-Gau commented Feb 14, 2024

jeromedockes commented Jan 23, 2024 •

edited by Remi-Gau

codecov bot commented Jan 23, 2024 •

edited

Remi-Gau commented Feb 13, 2024 •

edited