[ENH] Let _check_embedded_nifti_masker work with surface masker #4120

ymzayek · 2023-11-24T16:59:42Z

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

github-actions · 2023-11-24T16:59:55Z

👋 @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 · 2023-11-27T14:14:48Z

Codecov Report

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

Comparison is base (d6d822d) 91.81% compared to head (292f9bc) 91.85%.
Report is 5 commits behind head on main.

Files	Patch %	Lines
nilearn/decoding/space_net.py	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4120      +/-   ##
==========================================
+ Coverage   91.81%   91.85%   +0.03%     
==========================================
  Files         144      144              
  Lines       16172    16236      +64     
  Branches     3360     3381      +21     
==========================================
+ Hits        14849    14913      +64     
+ Misses        781      780       -1     
- Partials      542      543       +1

Flag	Coverage Δ
macos-latest_3.10_test_plotting	`91.67% <92.59%> (+0.03%)`	⬆️
macos-latest_3.11_test_plotting	`91.67% <92.59%> (+0.03%)`	⬆️
macos-latest_3.12_test_plotting	`91.67% <92.59%> (+0.03%)`	⬆️
macos-latest_3.8_test_plotting	`91.63% <92.59%> (+0.03%)`	⬆️
macos-latest_3.9_test_plotting	`91.64% <92.59%> (+0.03%)`	⬆️
ubuntu-latest_3.10_test_plotting	`91.67% <92.59%> (+0.03%)`	⬆️
ubuntu-latest_3.11_test_plotting	`91.67% <92.59%> (+0.03%)`	⬆️
ubuntu-latest_3.12_test_plotting	`91.67% <92.59%> (+0.03%)`	⬆️
ubuntu-latest_3.12_test_pre	`91.67% <92.59%> (+0.03%)`	⬆️
ubuntu-latest_3.8_test_min	`69.06% <92.59%> (-0.11%)`	⬇️
ubuntu-latest_3.8_test_plot_min	`91.39% <92.59%> (+0.03%)`	⬆️
ubuntu-latest_3.8_test_plotting	`91.63% <92.59%> (+0.03%)`	⬆️
ubuntu-latest_3.9_test_plotting	`91.64% <92.59%> (+0.03%)`	⬆️
windows-latest_3.10_test_plotting	`91.65% <92.59%> (+0.03%)`	⬆️
windows-latest_3.11_test_plotting	`91.65% <92.59%> (+0.03%)`	⬆️
windows-latest_3.12_test_plotting	`91.65% <92.59%> (+0.03%)`	⬆️
windows-latest_3.8_test_plotting	`91.61% <92.59%> (+0.03%)`	⬆️
windows-latest_3.9_test_plotting	`91.62% <92.59%> (+0.03%)`	⬆️

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 · 2023-11-27T16:09:50Z

nilearn/maskers/_masker_validation.py

 from .._utils.cache_mixin import _check_memory
 from .._utils.class_inspect import get_params
 from .multi_nifti_masker import MultiNiftiMasker
 from .nifti_masker import NiftiMasker


-def _check_embedded_nifti_masker(estimator, multi_subject=True):
+def _check_embedded_nifti_masker(estimator, masker_type="multi_nii"):


Can't we just remove this keyword argument completely and just set masker_type based on isinstance checks?

I agree with that. ATM the masker_type could be "multi_nii", 'surface', 'nifti', but this is fare from clear.

Remi-Gau · 2023-11-29T17:00:59Z

nilearn/_utils/__init__.py

-            "tests",
-            "externals",
-        }
+        modules_to_ignore = {"data", "tests", "externals", "conftest"}


functions in conftest.py should be ignored by default

Remi-Gau · 2023-11-29T17:01:31Z

nilearn/_utils/tests/test_common.py

@@ -34,7 +34,7 @@ def test_number_public_functions():
    If this is intentional, then the number should be updated in the test.
    Otherwise it means that the public API of nilearn has changed by mistake.
    """
-    assert len({_[0] for _ in all_functions()}) == 227
+    assert len({_[0] for _ in all_functions()}) == 205


consequence of ignoring functions in conftest.py

Remi-Gau · 2023-11-29T17:08:55Z

nilearn/conftest.py

@@ -12,6 +12,7 @@
 # we need to import these fixtures even if not used in this module
 from nilearn.datasets.tests._testing import request_mocker  # noqa: F401
 from nilearn.datasets.tests._testing import temp_nilearn_data_dir  # noqa: F401
+from nilearn.experimental.surface.tests.conftest import *  # noqa: F401, F403


I would not be opposed to move the fixture in nilearn/conftest.py rather than doing an "import all".
Any reasons why we should not do it?

Not a blocker for a merge.

Well we wanted to isolate the new surface module and related code while it's still being developed. But I don't see a way to do that for this PR. I don't like this solution either I just didn't want to rewrite the fixtures and this is temporary. I can instead just import the needed fixtures

OK let's leave it like this for now though maybe we should probably leave a comment to flag this for future us so that the temporary does not become too permanent.

I made the imports explicit and added a note.

awesome!!! thanks

Remi-Gau

Good with me

ymzayek · 2023-11-30T09:35:24Z

I have not added a test for space net yet but I'm thinking to do it in another PR. It might be more involved to get it to work since BaseSpaceNet fit method calls get_data on the mask img, which is not adapted to surface data. I'll open a follow-up PR after this is merged

bthirion

LGTM.

bthirion · 2023-11-27T17:53:50Z

nilearn/maskers/_masker_validation.py

 from .._utils.cache_mixin import _check_memory
 from .._utils.class_inspect import get_params
 from .multi_nifti_masker import MultiNiftiMasker
 from .nifti_masker import NiftiMasker


-def _check_embedded_nifti_masker(estimator, multi_subject=True):
+def _check_embedded_nifti_masker(estimator, masker_type="multi_nii"):


I agree with that. ATM the masker_type could be "multi_nii", 'surface', 'nifti', but this is fare from clear.

ymzayek · 2023-12-04T10:16:54Z

I agree with that. ATM the masker_type could be "multi_nii", 'surface', 'nifti', but this is fare from clear.

@bthirion it is stated in the docstring of this function and it is a private function in any case. What is supported in terms of the type of makser is also relayed to the functions or classes that use this function.

bthirion · 2023-12-04T21:42:28Z

Can be merged I think.

ymzayek added 2 commits November 24, 2023 17:50

Make SurfaceMasker valid for Decoder

b0df3c7

Remove restriction

9924d5b

ymzayek added 2 commits November 24, 2023 18:09

Fix space net

82abc5c

Fix test

661edbb

ymzayek requested review from bthirion and Remi-Gau November 27, 2023 16:06

ymzayek commented Nov 27, 2023

View reviewed changes

ymzayek and others added 5 commits November 28, 2023 11:07

Rename

936dc66

Add doctsrings and test

08feb77

Fix doctsrings

d585edd

Add to test

6dc9126

do not list conftest.py as part of the public functions

501fddd

Remi-Gau reviewed Nov 29, 2023

View reviewed changes

Remi-Gau approved these changes Nov 29, 2023

View reviewed changes

Make explicit imports

1261a6d

Move masker validation to _utils

6fe7aad

ymzayek added this to the release 0.11.0 milestone Nov 30, 2023

bthirion approved these changes Nov 30, 2023

View reviewed changes

ymzayek added 2 commits December 4, 2023 11:11

Fix multi case and comments

22b7dc2

Fix string

aaff642

Update docstring

292f9bc

ymzayek merged commit ef08959 into nilearn:main Dec 5, 2023
32 checks passed

ymzayek deleted the decoder_masker_validation branch December 5, 2023 08:54

This was referenced Jan 12, 2024

[ENH] Next steps for development on experimental surface module #4027

Open

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

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Let _check_embedded_nifti_masker work with surface masker #4120

[ENH] Let _check_embedded_nifti_masker work with surface masker #4120

ymzayek commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

codecov bot commented Nov 27, 2023 •

edited

ymzayek Nov 27, 2023

bthirion Nov 27, 2023

Remi-Gau Nov 29, 2023

Remi-Gau Nov 29, 2023

Remi-Gau Nov 29, 2023

Remi-Gau Nov 29, 2023

ymzayek Nov 30, 2023

Remi-Gau Nov 30, 2023

ymzayek Nov 30, 2023

Remi-Gau Nov 30, 2023

Remi-Gau left a comment

ymzayek commented Nov 30, 2023

bthirion left a comment

bthirion Nov 27, 2023

ymzayek commented Dec 4, 2023

bthirion commented Dec 4, 2023

[ENH] Let _check_embedded_nifti_masker work with surface masker #4120

[ENH] Let _check_embedded_nifti_masker work with surface masker #4120

Conversation

ymzayek commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

codecov bot commented Nov 27, 2023 • edited

Codecov Report

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

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 left a comment

Choose a reason for hiding this comment

ymzayek commented Nov 30, 2023

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymzayek commented Dec 4, 2023

bthirion commented Dec 4, 2023

codecov bot commented Nov 27, 2023 •

edited