nilearn icon indicating copy to clipboard operation
nilearn copied to clipboard

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

Open ymzayek opened this issue 1 year ago • 7 comments

  • Part of #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:

  • [x] Double check public class/method and function names
  • [x] 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
  • [x] Remove type hints
  • [x] Update doc/modules/surface.rst and remove experimental.rst
  • [x] Update experimental surface example
    • [ ] Integrate parts into stable examples (to do in follow-up PRs)
  • [x] 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

ymzayek avatar Jan 17 '24 15:01 ymzayek

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

github-actions[bot] avatar Jan 17 '24 15:01 github-actions[bot]

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 :warning:
nilearn/_utils/testing.py 66.66% 1 Missing and 1 partial :warning:
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%) :arrow_down:
macos-latest_3.11_test_plotting 91.82% <90.78%> (-0.04%) :arrow_down:
macos-latest_3.12_test_plotting 91.82% <90.78%> (-0.04%) :arrow_down:
macos-latest_3.8_test_plotting 91.78% <90.78%> (-0.04%) :arrow_down:
macos-latest_3.9_test_plotting 91.78% <90.78%> (-0.04%) :arrow_down:
ubuntu-latest_3.10_test_plotting 91.82% <90.78%> (-0.04%) :arrow_down:
ubuntu-latest_3.11_test_plotting 91.82% <90.78%> (?)
ubuntu-latest_3.12_test_plotting 91.82% <90.78%> (-0.04%) :arrow_down:
ubuntu-latest_3.12_test_pre 91.82% <90.78%> (-0.04%) :arrow_down:
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%) :arrow_down:
windows-latest_3.11_test_plotting 91.80% <90.78%> (-0.04%) :arrow_down:
windows-latest_3.12_test_plotting 91.80% <90.78%> (-0.04%) :arrow_down:
windows-latest_3.8_test_plotting 91.76% <90.78%> (-0.04%) :arrow_down:
windows-latest_3.9_test_plotting 91.76% <90.78%> (-0.04%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 17 '24 15:01 codecov[bot]

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 avatar Jan 24 '24 08:01 Remi-Gau

  • 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 avatar Jan 24 '24 09:01 Remi-Gau

  • blocked by #4235

let's integrate plotting first before moving out of experimental

Remi-Gau avatar Jan 24 '24 16:01 Remi-Gau

TODO

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

Remi-Gau avatar Jan 24 '24 18:01 Remi-Gau

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

jeromedockes avatar Jan 24 '24 19:01 jeromedockes

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: https://github.com/nilearn/nilearn/issues/4027

Remi-Gau avatar Jun 05 '24 09:06 Remi-Gau