[META] Next steps for development on experimental surface module

[ymzayek]

General

• split experimental package into: maskers, plotting, datasets to make them easier to later integrate into the rest of the code base

Plotting

• more plotting functions: make sure all surface plotting functions can work with the new surface object
  • Done in : [ENH] expand surface plotting with new surface object #4431
• bg_map of the new plotting functions can be a SurfaceImage instance
  • Done in [ENH] expand surface plotting with new surface object #4431

Maskers

• Implement extraction of signals from ROI and transforming region signals back to surface object
• Add report for surface masker
  • Done in: [ENH] add report for SurfaceMasker #4432
• From (SurfaceImage and maskers #3856 (comment)):
  • improve a bit the masker for the very frequent use case where we don't want to mask anything by avoiding to store the mask img
  • Implement a SurfaceVectorizer that just checks shapes and concatenates data but doesn't have a mask_img and doesn't mask

Decoder

• [ENH] Allow Decoder to work with surface objects #4205
• Update _check_embedded_nifti_masker to work with surface masker
  • Done in [ENH] Let _check_embedded_nifti_masker work with surface masker #4120

GLM

• BIDS compatibility: make it possible for first_level_from_bids to return surface images directly
• refactor example plotting.
  • See comment in [ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126 (comment)
  • Done in [ENH] expand surface plotting with new surface object #4431
• GLM report on surface
  • blocked by plotting [ENH] add plotting functions to the experimental surface module #4235
• immplement GLM on surface: [ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126

Plotting

• Adapting surf plotting functions for SurfaceImage [ENH] expand surface plotting with new surface object #4431

Documentation

• Add information about surface API to user guide
• Add surface terms to glossary [DOC] link surface terms to glossary #4161
• See [DOC] improve surface related documentation  #4152

Misc

• add functions to return fsaverage data (sulcal, curvature...) as SurfaceImage instance with specific mesh (pial, flat...)
  • Done in [ENH] expand surface plotting with new surface object #4431
• add possibility to create surface object directly from files
• add possibility to project volume to surface.
  • See comment in: [ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126 (comment)
• demonstrate how to save to files

Examples to adapt

• examples/01_plotting/plot_3d_map_to_surface_projection.py [ENH] expand surface plotting with new surface object #4431
• examples/01_plotting/plot_surf_atlas.py [ENH] expand surface plotting with new surface object #4431
• examples/01_plotting/plot_surf_stat_map.py [ENH] expand surface plotting with new surface object #4431
• examples/02_decoding/plot_haxby_searchlight_surface.py
• examples/04_glm_first_level/plot_localizer_surface_analysis.py [ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126
• examples/07_advanced/plot_surface_bids_analysis.py [ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126

Move experimental into stable

• 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?

PRs and issues made obsolete

List previous attempts of surface development that will ultimately be closed by this development.

• FirstLevelModel for volume on surface data #3265
• [ENH] Surface GLM - Support multiple runs in a GLM #3846
• [MRG] Update surface functions #2682
• [WIP] Support for SurfLabelsMasker (and a bit of broader Surface support) #2424
• SurfLabelsMasker #2423

Older issues for context

• improve support for surface data in nilearn? #2171

[Remi-Gau]

Copying some of the notes from our hackathon regarding writing surface data:

Writing surface images

• Separate meshes and textures into different gifti files
• Question about naming: should try and make reasonable defaults for that, yet leave the users the possibility to change names
  • Suggestion to go with with something BIDS like where the hemisphere:
    • https://bids-specification.readthedocs.io/en/latest/derivatives/introduction.html#gifti-surface-data-format
  • [hemi-{L|R}][_space-][_den-]_suffix.surf.gii
    • Wonder how much of those we can infer without having to keep track of the name of the input file…
    • Suffix is going to be almost impossible to have a sensible default for
    • Hemi specifies the hemisphere of a given file
    • Space for example “fsaverage”
    • Den for density, for example:
      • Den-10k for "10242 vertices per hemisphere (5th order icosahedron)"

[ymzayek]

@Remi-Gau we need to note that #4120 needs to be reverted if we release without the surface API.

In addition I'm thinking whether it would be better to develop the integration on a feature branch?

[Remi-Gau]

@Remi-Gau we need to note that #4120 needs to be reverted if we release without the surface API.

Good point

In addition I'm thinking whether it would be better to develop the integration on a feature branch?

We could try that indeed: to avoid those reversions, it would make sense.
Just need to make sure that we back merge main into it often to avoid diverging (though I suspect that if we work mostly on experimental stuff then we should be fine)

[bthirion]

What's preventing from releasing with the Surface API ? The plotting functions ?

[ymzayek]

I wanted to point that out in case we decide not to for whatever reason.

I'm happy to work on the plotting next week if @jeromedockes or @alexisthual point me to any notes or list them here that you made when discussing this if you do not have time to work on it.

[jeromedockes]

@Remi-Gau we need to note that #4120 needs to be reverted if we release without the surface API.

maybe we could avoid reverting it. to avoid importing the experimental module we can add some attribute for duck typing or even just check the class name instead of the isinstance checks in the check_embedded_nifti_masker and decoding functions. Once the module is no longer experimental we can change it back

[Remi-Gau]

As far as I can tell the SurfaceMasker does not include a generate_report method like the other maskers.

[ymzayek]

As far as I can tell the SurfaceMasker does not include a generate_report method like the other maskers.

I define it and raise NotImplemetedError in #4126

[ymzayek]

Oh meant that's for GLM but I think you can do the same for the masker for now

[jeromedockes]

Yasmin ***@***.***> writes:

> As far as I can tell the SurfaceMasker does not include a `generate_report` method like the other maskers. I define it and raise NotImplemetedError in #4126

Why is that better than not defining it?

[ymzayek]

@jeromedockes to raise a message with the error that it is not yet implemented and imply that it will be. But feel free to do otherwise

[Remi-Gau]

Why is that better than not defining it?

I would say mostly as signal to users (and future us) that this is coming.

[Remi-Gau]

met with @bthirion and @jeromedockes at the open office hours

Decisions:

• need to better develop the experimental aspects of the Surface features before we move it into the stable API
  • integrate plotting first
  • integrate surface GLM
  • integrate decoding
• so the next release will be a minor release instead of a major one

[bthirion]

... and for the time being, we represent meshes through dictionaries.

[Remi-Gau]

@bthirion @htwangtw @emdupre @alexisthual @ymzayek

Opinions needed

@jeromedockes and I chatted a bit more regarding the next steps.

Currently on main, the experimental module is imported in the following "non-experiemntal" places.

$ find nilearn -name '*.py' -exec grep -H "nilearn.experimental" {} \;

nilearn/decoding/space_net.py:from nilearn.experimental.surface import SurfaceMasker
nilearn/decoding/decoder.py:from nilearn.experimental.surface import SurfaceMasker
nilearn/decoding/tests/test_decoder.py:from nilearn.experimental.surface import SurfaceMasker
nilearn/_utils/masker_validation.py:from nilearn.experimental.surface import SurfaceMasker
nilearn/_utils/tests/test_masker_validation.py:from nilearn.experimental.surface import SurfaceMasker

This leads to the warning from experimental module to be thrown when users use non-experimental part of the codebase.

nilearn/nilearn/experimental/__init__.py

Line 10 in cfda9ff

warnings.warn(

We feel that rather reverting #4120 (see #4027 (comment)) it would be OK to get rid of the experimental warning, assuming that users wanting to use the experimental features would literal have to type "experimental".

This could be replaced with a additional warning in the doc: https://nilearn.github.io/dev/modules/experimental.html and in the coming release notes.

How do others feel about this?

[Remi-Gau]

TODO:

• modified examples from [ENH] Adapt FirstLevelModel to work with surface data objects from new API #4126 should be copied as a new experimental example and the the "old" example should mention the new experimental example

A similar approach (copy to experimental + mention new experimental example in old version) should be be used for other examples where the new surface API can be applied.

• update top message of this issue with a list of examples to be modified with the new surface API

[jeromedockes]

would literal have to type "experimental".

indeed that is more likely to catch their attention than any warning

[jeromedockes]

... and for the time being, we represent meshes through dictionaries.

or rather collections of meshes containing one or more hemispheres. each individual mesh is still represented by a Mesh instance

[jeromedockes]

I would say mostly as signal to users (and future us) that this is coming.

ok my 2 cents is stating it exists by defining it and then surprise it doesn't when we call it is not super helpful, and not defining it at all is less work but it's not important either way

[Remi-Gau]

Adding a section in the top messages to mention issues or PR that will be made obsolete or superseded by the ones created here