[FIX] check data format passed to surface plotting functions #4323

Remi-Gau · 2024-03-18T15:55:57Z

Closes passing nifti to surface plotting functions fails #4312

Changes proposed in this pull request:

extract a function to check extension of files passed to surface plotting functions
make sure that surface plotting functions (except plot_img_on_surf and view_img_on_surf) throw errors when nifti files are passed
add appropriate tests

github-actions · 2024-03-18T16:09:50Z

👋 @Remi-Gau 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 · 2024-03-18T16:38:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.13%. Comparing base (abb80ff) to head (b501218).
Report is 29 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4323      +/-   ##
==========================================
+ Coverage   91.85%   92.13%   +0.28%     
==========================================
  Files         144      143       -1     
  Lines       16419    16462      +43     
  Branches     3434     3452      +18     
==========================================
+ Hits        15082    15168      +86     
+ Misses        792      749      -43     
  Partials      545      545

Flag	Coverage Δ
macos-latest_3.10_test_plotting	`91.93% <100.00%> (?)`
macos-latest_3.11_test_plotting	`91.93% <100.00%> (+0.07%)`	⬆️
macos-latest_3.8_test_plotting	`91.89% <100.00%> (?)`
macos-latest_3.9_test_plotting	`91.89% <100.00%> (?)`
ubuntu-latest_3.10_test_plotting	`91.93% <100.00%> (+0.07%)`	⬆️
ubuntu-latest_3.11_test_plotting	`91.93% <100.00%> (?)`
ubuntu-latest_3.12_test_plotting	`91.93% <100.00%> (?)`
ubuntu-latest_3.12_test_pre	`91.93% <100.00%> (?)`
ubuntu-latest_3.8_test_min	`68.78% <38.46%> (?)`
ubuntu-latest_3.8_test_plot_min	`91.59% <100.00%> (?)`
ubuntu-latest_3.8_test_plotting	`91.89% <100.00%> (?)`
ubuntu-latest_3.9_test_plotting	`91.89% <100.00%> (?)`
windows-latest_3.10_test_plotting	`91.90% <100.00%> (?)`
windows-latest_3.11_test_plotting	`91.90% <100.00%> (?)`
windows-latest_3.12_test_plotting	`91.90% <100.00%> (?)`
windows-latest_3.8_test_plotting	`91.86% <100.00%> (?)`
windows-latest_3.9_test_plotting	`91.87% <100.00%> (?)`

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.

Remi-Gau · 2024-03-18T18:15:37Z

nilearn/plotting/tests/test_surf_plotting.py

@@ -879,101 +879,102 @@ def test_plot_surf_roi_colorbar_vmin_equal_across_engines(kwargs):


 def test_plot_img_on_surf_hemispheres_and_orientations(img_3d_mni):
-    nii = img_3d_mni


Several tests had this futile reassigning of the fixture, cleaning it up here.

Remi-Gau · 2024-03-18T18:16:28Z

nilearn/plotting/surf_plotting.py

@@ -1362,7 +1380,7 @@ def plot_img_on_surf(stat_map, surf_mesh='fsaverage5', mask_img=None,

    Parameters
    ----------
-    stat_map : str or 3D Niimg-like object
+    stat_map : :obj:`str` or :class:`pathlib.Path` or 3D Niimg-like object


Tested that the code works with both string and path so updated the doc.

On that note, it may be worth checking in the public API that users can also pass a Path in places where they can pass a string to a file.

I have not checked but it seems that many functions require a string and that Paths are not supported (or at least the doc string is not necessarily explicit - or I missed it).

May be done as some internal refactoring to rely more on pathlib rather than os (where relevant) - to be discussed.

Yes, we should open an issue and address that later.

Remi-Gau · 2024-03-18T18:17:09Z

nilearn/conftest.py

+def img_3d_mni_as_file(tmp_path):
+    """Return path to a random 3D Nifti1Image in MNI space saved to disk."""
+    filename = tmp_path / "img.nii"
+    nb.save(_img_3d_mni(), filename)
+    return filename


Added in this as I needed it but probably reusable in other tests.

Remi-Gau · 2024-03-18T19:08:54Z

nilearn/plotting/tests/test_html_surface.py

-def _get_img():
+@pytest.fixture(scope="session")
+def mni152_template_res_2():


refactor this as fixture to help reusing it

Remi-Gau · 2024-03-18T19:09:25Z

nilearn/plotting/tests/test_html_surface.py

+def test_view_img_on_surf_errors(img_3d_mni):
+    with pytest.raises(DimensionError):
+        view_img_on_surf([img_3d_mni, img_3d_mni])


extract errors in a separate test

Remi-Gau · 2024-03-18T19:09:57Z

nilearn/plotting/tests/test_html_surface.py

+def test_view_img_on_surf_input_as_file(img_3d_mni_as_file):
+    view_img_on_surf(img_3d_mni_as_file)
+    view_img_on_surf(str(img_3d_mni_as_file))


check that files are accepted as string or paths

Remi-Gau · 2024-03-18T19:10:34Z

nilearn/plotting/tests/test_html_surface.py

-    img = _get_img()
-    surfaces = datasets.fetch_surf_fsaverage()
-    html = html_surface.view_img_on_surf(img, threshold='92.3%')
+def test_view_img_on_surf(mni152_template_res_2):


sorry for the horrible diff on this part but this test was pretty unreadable

bthirion

Did not spot any issue. Thx.

bthirion · 2024-03-18T22:22:28Z

nilearn/plotting/surf_plotting.py

@@ -1362,7 +1380,7 @@ def plot_img_on_surf(stat_map, surf_mesh='fsaverage5', mask_img=None,

    Parameters
    ----------
-    stat_map : str or 3D Niimg-like object
+    stat_map : :obj:`str` or :class:`pathlib.Path` or 3D Niimg-like object


Yes, we should open an issue and address that later.

nilearn/conftest.py

start refactoring

6e6fa89

add extension checks to surface plotting functions

1f541cb

implement and add tests

2b10c4b

Remi-Gau commented Mar 18, 2024

View reviewed changes

Remi-Gau added 2 commits March 18, 2024 14:31

isort fix

8c9d161

apply to view_img_on_surf

0025994

Remi-Gau commented Mar 18, 2024

View reviewed changes

flake8

b501218

Remi-Gau marked this pull request as ready for review March 18, 2024 19:20

Remi-Gau changed the title ~~[WIP][FIX] check data format passed to surface plotting functions~~ [WIP] check data format passed to surface plotting functions Mar 18, 2024

Remi-Gau changed the title ~~[WIP] check data format passed to surface plotting functions~~ [FIX] check data format passed to surface plotting functions Mar 18, 2024

bthirion approved these changes Mar 18, 2024

View reviewed changes

Remi-Gau requested a review from man-shu March 18, 2024 22:57

man-shu reviewed Mar 19, 2024

View reviewed changes

nilearn/conftest.py Show resolved Hide resolved

man-shu approved these changes Mar 19, 2024

View reviewed changes

Remi-Gau merged commit 4691c77 into nilearn:main Mar 19, 2024
33 of 34 checks passed

Remi-Gau deleted the fix/4312 branch March 19, 2024 13:00

Remi-Gau mentioned this pull request Mar 19, 2024

Extend/improve support for pathlib.Path #4327

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] check data format passed to surface plotting functions #4323

[FIX] check data format passed to surface plotting functions #4323

Remi-Gau commented Mar 18, 2024 •

edited

github-actions bot commented Mar 18, 2024

codecov bot commented Mar 18, 2024 •

edited

Remi-Gau Mar 18, 2024

Remi-Gau Mar 18, 2024

Remi-Gau Mar 18, 2024

bthirion Mar 18, 2024

Remi-Gau Mar 18, 2024

Remi-Gau Mar 18, 2024

Remi-Gau Mar 18, 2024

Remi-Gau Mar 18, 2024

Remi-Gau Mar 18, 2024

bthirion left a comment

bthirion Mar 18, 2024

		@@ -879,101 +879,102 @@ def test_plot_surf_roi_colorbar_vmin_equal_across_engines(kwargs):


		def test_plot_img_on_surf_hemispheres_and_orientations(img_3d_mni):
		nii = img_3d_mni

[FIX] check data format passed to surface plotting functions #4323

[FIX] check data format passed to surface plotting functions #4323

Conversation

Remi-Gau commented Mar 18, 2024 • edited

github-actions bot commented Mar 18, 2024

codecov bot commented Mar 18, 2024 • 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

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Mar 18, 2024 •

edited

codecov bot commented Mar 18, 2024 •

edited