First draft of surface stuff after discussion during meetings & drop-in hours #3799
Conversation
|
👋 @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.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
| ) | ||
|
|
||
|
|
||
| def load_fsaverage(mesh_name: str = "fsaverage5") -> Dict[str, PolyMesh]: |
There was a problem hiding this comment.
Maybe add argument for mesh type so that not all meshes are loaded
There was a problem hiding this comment.
right, I remember we thought about that. but I wonder if it will result in user having to call that function several times, as it is quite frequent to need 2 or 3 of them -- eg pial and white matter for volume to surface projection, then inflated for plotting? but at the same time when only 1 is needed adding the argument would make it faster
| return _io.read_mesh(self._file)["faces"] | ||
|
|
||
|
|
||
| class InMemoryMesh(Mesh): |
There was a problem hiding this comment.
Why not just expand the type of input for class Mesh since load_surf_mesh that's called in read_mesh takes in loaded arrays
There was a problem hiding this comment.
for now it uses load_surf_mesh just so we can focus on the surface image and maskers first and deal with the io later. but I think it might simplify things to have a distinction between files on disk and in-memory meshes, to avoid (i) functions like load_surf_mesh that have parameters that can be of several completely different types and (ii) a surface image that has several redundant/alternative attributes, some of which can be missing, and always having to deal with those multiple possible configurations
|
|
||
|
|
||
| @dataclasses.dataclass | ||
| class SurfaceImage: |
There was a problem hiding this comment.
Does it happen that an analysis pipeline uses just one hemisphere? If so and it's common we can consider PolyData keys to also map to a path and then set a method within this class to load a specific hemisphere array
There was a problem hiding this comment.
I think we will often need all the data, unlike the meshes which we won't use most of the time, so I'd rather keep things a bit simpler for now
There was a problem hiding this comment.
btw we can easily have a SurfaceImage that contains only one hemisphere (the polymesh and polydata would have just 1 element each). that can be done when creating the image or if it's useful we could have methods to extract one part of the image something like full_img.get_part("left_hemisphere")
|
@jeromedockes |
| mask_img_: SurfaceImage | None | ||
| output_dimension_: int | None | ||
|
|
||
| def _fit_mask_img(self, img: SurfaceImage) -> None: |
There was a problem hiding this comment.
I don't understand what to expect from this method. A short docstring could help.
| self._coordinates = coordinates | ||
| self._faces = faces | ||
| self.n_vertices = coordinates.shape[0] | ||
|
|
There was a problem hiding this comment.
Do we need some checks that faces and coordinates are consistent ?
tests are still failing @ymzayek and I suppose this is because of the monkeypatch in the example this leads to a more generic question - that we don't have to answer right now but it would be good to get to it at some point: where do we keep examples for experimental features? quick look at scikit-learn shows that there is doc for the experimental features but not necessarily examples. https://scikit-learn.org/stable/metadata_routing.html#metadata-routing possibility: have the example in our examples folder but do not include it in the doc build. |
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #3799 +/- ##
==========================================
- Coverage 91.59% 90.95% -0.65%
==========================================
Files 134 139 +5
Lines 15694 15874 +180
Branches 3270 3294 +24
==========================================
+ Hits 14375 14438 +63
- Misses 770 888 +118
+ Partials 549 548 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
as discussed with @Remi-Gau and @ymzayek
this adds the surface image and maskers module in a nilearn.experimental module
for now this is just a draft of the basic functionality, some features are missing as well as tests and an example
also it doesn't pass the pre-commit checks (missing docstrings)
the file in tests/example.py is the notebook that was used for discussion and is also in nilearn_sandbox and it should run without issues