[WIP][ENH] Move surface image API from experimental to stable API
- Part of #4027
Changes proposed in this pull request:
- Changes compared to
experimental.surface- Fetchers moved to datasets respectively to
atlas,func, andstructfetch_destrieuxrenamedload_sample_atlas_surf_destrieuxfetch_nkirenamedload_sample_surf_nki_enhancedload_fsaveragerenamedload_surf_fsaverage
mesh_nameparameter ofload_surf_fsaveragerenamedmeshfor consistency withfetch_surf_fsaverageMeshbase class renamed_MeshMeshalready exists as namedtuple which we will probably deprecate
- Fetchers moved to datasets respectively to
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._iotosurface._io
- At the moment just copied over wrappers from
- [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 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 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.
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._iotosurface._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
- 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.
- blocked by #4235
let's integrate plotting first before moving out of experimental
TODO
- [ ] extract the fixture renaming or turning into functions out of this PR.
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
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