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

ymzayek · 2024-01-17T15:13:11Z

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

Changes proposed in this pull request:

Changes compared to experimental.surface
- Fetchers moved to datasets respectively to atlas, func, and struct
  - fetch_destrieux renamed load_sample_atlas_surf_destrieux
  - fetch_nki renamed load_sample_surf_nki_enhanced
  - load_fsaverage renamed load_surf_fsaverage
- mesh_name parameter of load_surf_fsaverage renamed mesh for consistency with fetch_surf_fsaverage
- Mesh base class renamed _Mesh
  - Mesh already exists as namedtuple which we will probably deprecate

TODO:

Double check public class/method and function names
Double check/improve docstrings (still need review of course)
How to handle I/O and provide some public functions to replace surface.py loaders
- At the moment just copied over wrappers from experimental.surface._io to surface._io
Remove type hints
Update doc/modules/surface.rst and remove experimental.rst
Update experimental surface example
- Integrate parts into stable examples (to do in follow-up PRs)
Increase testing
What to deprecate from old surface.py API?

The following PRs will depend on merging this one:
#4126
#4205
#4120 - already merged, relevant imports are updated in this PR

github-actions · 2024-01-17T15:13:27Z

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

codecov · 2024-01-17T15:24:16Z

Codecov Report

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

Comparison is base (cfda9ff) 91.87% compared to head (048c744) 92.03%.

Files	Patch %	Lines
nilearn/datasets/func.py	64.28%	4 Missing and 1 partial ⚠️
nilearn/_utils/testing.py	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4229      +/-   ##
==========================================
+ Coverage   91.87%   92.03%   +0.16%     
==========================================
  Files         144      136       -8     
  Lines       16421    16217     -204     
  Branches     3434     3409      -25     
==========================================
- Hits        15086    14925     -161     
+ Misses        792      749      -43     
  Partials      543      543

Flag	Coverage Δ
macos-latest_3.10_test_plotting	`91.82% <90.78%> (-0.04%)`	⬇️
macos-latest_3.11_test_plotting	`91.82% <90.78%> (-0.04%)`	⬇️
macos-latest_3.12_test_plotting	`91.82% <90.78%> (-0.04%)`	⬇️
macos-latest_3.8_test_plotting	`91.78% <90.78%> (-0.04%)`	⬇️
macos-latest_3.9_test_plotting	`91.78% <90.78%> (-0.04%)`	⬇️
ubuntu-latest_3.10_test_plotting	`91.82% <90.78%> (-0.04%)`	⬇️
ubuntu-latest_3.11_test_plotting	`91.82% <90.78%> (?)`
ubuntu-latest_3.12_test_plotting	`91.82% <90.78%> (-0.04%)`	⬇️
ubuntu-latest_3.12_test_pre	`91.82% <90.78%> (-0.04%)`	⬇️
ubuntu-latest_3.8_test_min	`68.62% <90.78%> (?)`
ubuntu-latest_3.8_test_plot_min	`91.53% <90.78%> (?)`
ubuntu-latest_3.8_test_plotting	`91.78% <90.78%> (?)`
ubuntu-latest_3.9_test_plotting	`91.78% <90.78%> (?)`
windows-latest_3.10_test_plotting	`91.80% <90.78%> (-0.04%)`	⬇️
windows-latest_3.11_test_plotting	`91.80% <90.78%> (-0.04%)`	⬇️
windows-latest_3.12_test_plotting	`91.80% <90.78%> (-0.04%)`	⬇️
windows-latest_3.8_test_plotting	`91.76% <90.78%> (-0.04%)`	⬇️
windows-latest_3.9_test_plotting	`91.76% <90.78%> (-0.04%)`	⬇️

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.

ymzayek · 2024-01-17T15:28:51Z

nilearn/surface/surface_image.py


 PolyData = Dict[str, np.ndarray]


-class Mesh(abc.ABC):
+class _Mesh(abc.ABC):


If we want this to be a public class then need to deprecate surface.py's Mesh named tuple right away. Either way, we will need to deprecated the named tuples

Probably worth pinning this one for later.

bthirion

looks good so far.

nilearn/conftest.py

nilearn/datasets/atlas.py

nilearn/datasets/func.py

nilearn/conftest.py

Remi-Gau · 2024-01-23T09:11:11Z

examples/08_experimental/plot_surface_image_and_maskers.py

TODO

move this file out of the experimental example

Do in a follow up PR after we have dealt with:

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

nilearn/conftest.py

Remi-Gau · 2024-01-24T08:16:37Z

How to handle I/O and provide some public functions to replace surface.py loaders

At the moment just copied over wrappers from experimental.surface._io to surface._io

I think I would keep this for another PR probably one with example that shows basic "loading" of surface images because at the moment how to compose those SurfaceImage instances is hidden in our dataset utility functions like nilearn.datasets.func.load_sample_surf_nki_enhanced

Remi-Gau · 2024-01-24T08:28:32Z

nilearn/surface/surface_image.py


-    n_vertices: int
+    n_vertices: ...


OK I am sorry but I am putting types back in here because ... is just worse than mentioning the type.

Fair. I wasn't sure what to do about these so it's a bit unfinished. I think you should add back PolyMesh and PolyData as well

No worries. I was just confused as to why we were "ellisping" here.

In general, still sad we are removing type hints that were already here though.

you could remove the whole line or leave the type.

In general, still sad we are removing type hints that were already here though.

if they are complete enough within a sub-package that this part can successfully pass the mypy check we could leave them. what I would like to avoid is broken type hints, which is bound to happen if we don't run mypy in the ci

Remi-Gau · 2024-01-24T09:12:16Z

nilearn/maskers/surface_masker.py

+    # TODO check attribute names after PR 3761
+    # and harmonize with volume labels masker if necessary.
+    # https://github.com/nilearn/nilearn/pull/3761
+    def __init__(self, labels_img, label_names=None):


TODO in a follow up PR

Remi-Gau · 2024-01-24T09:19:02Z

What to deprecate from old surface.py API?

For the moment I would deprecate as little as possible: have both APIs coexist in the next release and then slowly start redirect to the new API after that.

Remi-Gau · 2024-01-24T16:11:24Z

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

let's integrate plotting first before moving out of experimental

Remi-Gau · 2024-01-24T18:16:08Z

TODO

extract the fixture renaming or turning into functions out of this PR.

jeromedockes · 2024-01-24T19:07:43Z

yeah but for one of those I was getting an "empty file" error: will try to investigate a bit more

there is some documentation in https://github.com/nilearn/nilearn/blob/main/nilearn/datasets/tests/_testing.py but it probably needs to be expanded. If you need help figuring out what this thing does LMK

bthirion

There remain a couple of "to fix" and codecov complaints (maybe wrong ?)
but I don't see anything fishy with the PR.
Nevertheless, we're not going to merge it shortly.

Remi-Gau · 2024-06-05T09:25:05Z

Closing this one as it will be easier to start it from scratch rather than fixing the merge conflicts.

Move the list of TODOs in here: [META] Next steps for development on experimental surface module #4027

ymzayek added 7 commits January 17, 2024 13:12

Moving surface api out of experimental

5e00ef2

Move tests

e52f5d3

Move datasets functions

b7e495f

Move conftest fixtures

c35d0f4

Fix some imports

f074ab4

Fix some circular imports

b6505c9

Fix some tests

f868232

ymzayek added the Blocker label Jan 17, 2024

ymzayek commented Jan 17, 2024

View reviewed changes

ymzayek added 5 commits January 17, 2024 17:18

Update example

1c8edb6

Update doc modules

2ff6975

Update doc and example

d070fef

Remove typing from surface maskers

1298a44

Remove typing from surface

6625dd8

bthirion reviewed Jan 17, 2024

View reviewed changes

nilearn/conftest.py Outdated Show resolved Hide resolved

ymzayek mentioned this pull request Jan 18, 2024

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

Merged

2 tasks

ymzayek added 2 commits January 18, 2024 15:31

Correct fixture docstring

6e7720e

Update docstrings

26868aa

ymzayek commented Jan 22, 2024

View reviewed changes

nilearn/datasets/atlas.py Outdated Show resolved Hide resolved

ymzayek commented Jan 22, 2024

View reviewed changes

nilearn/datasets/func.py Outdated Show resolved Hide resolved

ymzayek and others added 2 commits January 22, 2024 11:43

Merge branch 'main' into surface-image-stable

3f1d92c

Fix doc build

85990b6

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

Remi-Gau reviewed Jan 23, 2024

View reviewed changes

nilearn/conftest.py Outdated Show resolved Hide resolved

Remi-Gau reviewed Jan 23, 2024

View reviewed changes

nilearn/conftest.py Outdated Show resolved Hide resolved

Remi-Gau reviewed Jan 23, 2024

View reviewed changes

Remi-Gau added 2 commits January 23, 2024 10:25

load all meshes fsaverage

1fbe5cc

add xfail smoke test

f2a9aa9

Remi-Gau reviewed Jan 23, 2024

View reviewed changes

nilearn/conftest.py Outdated Show resolved Hide resolved

Remi-Gau reviewed Jan 23, 2024

View reviewed changes

nilearn/conftest.py Outdated Show resolved Hide resolved

Remi-Gau added 3 commits January 23, 2024 13:35

update doc

ab130cb

rename functions

004c4e5

turn fixtures into functions

572d6d2

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

Remi-Gau mentioned this pull request Jan 24, 2024

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

Merged

9 tasks

Remi-Gau reviewed Jan 24, 2024

View reviewed changes

Remi-Gau added 2 commits January 24, 2024 10:03

add types for data classes and update docstring

cf14f56

fix type for 3.8

85724d1

Remi-Gau reviewed Jan 24, 2024

View reviewed changes

keep future annotations

a93e803

update changelog

048c744

Remi-Gau changed the title ~~[WIP][ENH] Move surface image API from experimental to stable API~~ [ENH] Move surface image API from experimental to stable API Jan 24, 2024

Remi-Gau marked this pull request as ready for review January 24, 2024 09:21

Remi-Gau requested review from bthirion and jeromedockes January 24, 2024 09:25

Remi-Gau added the Blocked label Jan 24, 2024

Remi-Gau changed the title ~~[ENH] Move surface image API from experimental to stable API~~ [WIP][ENH] Move surface image API from experimental to stable API Jan 24, 2024

Remi-Gau marked this pull request as draft January 24, 2024 16:10

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

bthirion reviewed Jan 24, 2024

View reviewed changes

Remi-Gau closed this Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ymzayek commented Jan 17, 2024 •

edited by Remi-Gau

github-actions bot commented Jan 17, 2024

codecov bot commented Jan 17, 2024 •

edited

ymzayek Jan 17, 2024

Remi-Gau Jan 24, 2024

bthirion left a comment

Remi-Gau Jan 23, 2024 •

edited

Remi-Gau Jan 24, 2024

Remi-Gau commented Jan 24, 2024 •

edited

Remi-Gau Jan 24, 2024

ymzayek Jan 24, 2024

Remi-Gau Jan 24, 2024

jeromedockes Jan 24, 2024

Remi-Gau Jan 24, 2024

Remi-Gau commented Jan 24, 2024

Remi-Gau commented Jan 24, 2024

Remi-Gau commented Jan 24, 2024 •

edited

jeromedockes commented Jan 24, 2024 via email

bthirion left a comment

Remi-Gau commented Jun 5, 2024

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

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

Conversation

ymzayek commented Jan 17, 2024 • edited by Remi-Gau

github-actions bot commented Jan 17, 2024

codecov bot commented Jan 17, 2024 • edited

Codecov Report

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

Remi-Gau Jan 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Jan 24, 2024 • 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

Remi-Gau commented Jan 24, 2024

Remi-Gau commented Jan 24, 2024

Remi-Gau commented Jan 24, 2024 • edited

jeromedockes commented Jan 24, 2024 via email

bthirion left a comment

Choose a reason for hiding this comment

Remi-Gau commented Jun 5, 2024

ymzayek commented Jan 17, 2024 •

edited by Remi-Gau

codecov bot commented Jan 17, 2024 •

edited

Remi-Gau Jan 23, 2024 •

edited

Remi-Gau commented Jan 24, 2024 •

edited

Remi-Gau commented Jan 24, 2024 •

edited