Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion nilearn/_utils/tests/test_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()}) == 240
assert len({_[0] for _ in all_functions()}) == 241


def test_number_public_classes():
Expand Down
9 changes: 9 additions & 0 deletions nilearn/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import warnings

import nibabel
import nibabel as nb
import numpy as np
import pytest
from nibabel import Nifti1Image
Expand Down Expand Up @@ -243,6 +244,14 @@ def img_3d_mni():
return _img_3d_mni()


@pytest.fixture()
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"
man-shu marked this conversation as resolved.
Show resolved Hide resolved
nb.save(_img_3d_mni(), filename)
return filename
Comment on lines +248 to +252
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



def _img_3d_zeros(shape=_shape_3d_default(), affine=_affine_eye()):
"""Return a default zeros filled 3D Nifti1Image (identity affine).

Expand Down
45 changes: 33 additions & 12 deletions nilearn/plotting/surf_plotting.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,20 @@
from nilearn.plotting.img_plotting import get_colorbar_and_data_ranges
from nilearn.plotting.js_plotting_utils import colorscale
from nilearn.surface import load_surf_data, load_surf_mesh, vol_to_surf
from nilearn.surface.surface import check_mesh
from nilearn.surface.surface import (

Check warning on line 26 in nilearn/plotting/surf_plotting.py

View check run for this annotation

Codecov / codecov/patch

nilearn/plotting/surf_plotting.py#L26

Added line #L26 was not covered by tests
FREESURFER_DATA_EXTENSIONS,
check_extensions,
check_mesh,
)

VALID_VIEWS = "anterior", "posterior", "medial", "lateral", "dorsal", "ventral"
VALID_HEMISPHERES = "left", "right"

# subset of data format extensions supported
DATA_EXTENSIONS = ("gii",

Check warning on line 36 in nilearn/plotting/surf_plotting.py

View check run for this annotation

Codecov / codecov/patch

nilearn/plotting/surf_plotting.py#L36

Added line #L36 was not covered by tests
"gii.gz",
"mgz",)


MATPLOTLIB_VIEWS = {
"left": {
Expand Down Expand Up @@ -813,6 +822,9 @@
"cbar_vmax": cbar_vmax,
"alpha": alpha
}

check_extensions(surf_map, DATA_EXTENSIONS, FREESURFER_DATA_EXTENSIONS)

Check warning on line 826 in nilearn/plotting/surf_plotting.py

View check run for this annotation

Codecov / codecov/patch

nilearn/plotting/surf_plotting.py#L826

Added line #L826 was not covered by tests

if engine == 'plotly':
for parameter, value in parameters_not_implemented_in_plotly.items():
if value is not None:
Expand Down Expand Up @@ -907,10 +919,12 @@
of the :term:`mesh` :term:`faces`.

roi_map : str or numpy.ndarray or list of numpy.ndarray
ROI map to be displayed on the surface mesh, can be a file
(valid formats are .gii, .mgz, .nii, .nii.gz, or Freesurfer specific
files such as .annot or .label), or
a Numpy array with a value for each :term:`vertex` of the surf_mesh.
ROI map to be displayed on the surface mesh,
can be a file
(valid formats are .gii, .mgz, or
Freesurfer specific files such as
.thickness, .area, .curv, .sulc, .annot, .label) or
a Numpy array with a value for each :term:`vertex` of the `surf_mesh`.
The value at each :term:`vertex` one inside the ROI
and zero inside ROI,
or an integer giving the label number for atlases.
Expand Down Expand Up @@ -971,7 +985,10 @@
_ = plot_surf(surf_mesh, axes=axes, **kwargs)

_, faces = load_surf_mesh(surf_mesh)

check_extensions(roi_map, DATA_EXTENSIONS, FREESURFER_DATA_EXTENSIONS)

Check warning on line 989 in nilearn/plotting/surf_plotting.py

View check run for this annotation

Codecov / codecov/patch

nilearn/plotting/surf_plotting.py#L989

Added line #L989 was not covered by tests
roi = load_surf_data(roi_map)

if levels is None:
levels = np.unique(roi_map)
if colors is None:
Expand Down Expand Up @@ -1058,9 +1075,10 @@

stat_map : str or numpy.ndarray
Statistical map to be displayed on the surface :term:`mesh`,
can be a file (valid formats are .gii, .mgz, .nii, .nii.gz, or
Freesurfer specific files such as .thickness, .area, .curv,
.sulc, .annot, .label) or
can be a file
(valid formats are .gii, .mgz, or
Freesurfer specific files such as
.thickness, .area, .curv, .sulc, .annot, .label) or
a Numpy array with a value for each :term:`vertex` of the `surf_mesh`.

bg_map : str or numpy.ndarray, optional
Expand Down Expand Up @@ -1183,6 +1201,7 @@
nilearn.surface.vol_to_surf : For info on the generation of surfaces.

"""
check_extensions(stat_map, DATA_EXTENSIONS, FREESURFER_DATA_EXTENSIONS)

Check warning on line 1204 in nilearn/plotting/surf_plotting.py

View check run for this annotation

Codecov / codecov/patch

nilearn/plotting/surf_plotting.py#L1204

Added line #L1204 was not covered by tests
loaded_stat_map = load_surf_data(stat_map)

# Call get_colorbar_and_data_ranges to derive symmetric vmin, vmax
Expand Down Expand Up @@ -1362,7 +1381,7 @@

Parameters
----------
stat_map : str or 3D Niimg-like object
stat_map : :obj:`str` or :class:`pathlib.Path` or 3D Niimg-like object
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

See :ref:`extracting_data`.

surf_mesh : str, dict, or None, default='fsaverage5'
Expand Down Expand Up @@ -1578,8 +1597,9 @@
roi_map : str or numpy.ndarray or list of numpy.ndarray
ROI map to be displayed on the surface :term:`mesh`,
can be a file
(valid formats are .gii, .mgz, .nii, .nii.gz, or Freesurfer specific
files such as .annot or .label), or
(valid formats are .gii, .mgz, or
Freesurfer specific files such as
.thickness, .area, .curv, .sulc, .annot, .label) or
a Numpy array with a value for each :term:`vertex` of the `surf_mesh`.
The value at each vertex one inside the ROI and zero inside ROI, or an
integer giving the label number for atlases.
Expand Down Expand Up @@ -1695,8 +1715,9 @@

# preload roi and mesh to determine vmin, vmax and give more useful error
# messages in case of wrong inputs

check_extensions(roi_map, DATA_EXTENSIONS, FREESURFER_DATA_EXTENSIONS)

Check warning on line 1718 in nilearn/plotting/surf_plotting.py

View check run for this annotation

Codecov / codecov/patch

nilearn/plotting/surf_plotting.py#L1718

Added line #L1718 was not covered by tests
roi = load_surf_data(roi_map)

idx_not_na = ~np.isnan(roi)
if vmin is None:
vmin = np.nanmin(roi)
Expand Down
78 changes: 50 additions & 28 deletions nilearn/plotting/tests/test_html_surface.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@
from nilearn.datasets import fetch_surf_fsaverage
from nilearn.image import get_data
from nilearn.plotting import html_surface
from nilearn.plotting.html_surface import view_img_on_surf
from nilearn.plotting.js_plotting_utils import decode

from .test_js_plotting_utils import check_colors, check_html


def _get_img():
@pytest.fixture(scope="session")
def mni152_template_res_2():
Comment on lines -16 to +18
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactor this as fixture to help reusing it

return datasets.load_mni152_template(resolution=2)


Expand Down Expand Up @@ -118,10 +120,10 @@ def test_one_mesh_info():
html_surface.one_mesh_info(surf_map, mesh)


def test_full_brain_info():
def test_full_brain_info(mni152_template_res_2):
surfaces = datasets.fetch_surf_fsaverage()
img = _get_img()
info = html_surface._full_brain_info(img, surfaces)

info = html_surface._full_brain_info(mni152_template_res_2, surfaces)
check_colors(info['colorscale'])
assert {'pial_left', 'pial_right',
'inflated_left', 'inflated_right',
Expand All @@ -145,22 +147,21 @@ def test_full_brain_info():
"_full_brain_info. Using the deprecated name will raise an error "
"in release 0.13",
):
html_surface.full_brain_info(img)
html_surface.full_brain_info(mni152_template_res_2)


def test_fill_html_template():
def test_fill_html_template(mni152_template_res_2):
fsaverage = fetch_surf_fsaverage()
mesh = surface.load_surf_mesh(fsaverage['pial_right'])
surf_map = mesh[0][:, 0]
img = _get_img()
info = html_surface._one_mesh_info(
surf_map, fsaverage['pial_right'], '90%', black_bg=True,
bg_map=fsaverage['sulc_right'])
info["title"] = None
html = html_surface._fill_html_template(info, embed_js=False)
check_html(html)
assert "jquery.min.js" in html.html
info = html_surface._full_brain_info(img)
info = html_surface._full_brain_info(mni152_template_res_2)
info["title"] = None
html = html_surface._fill_html_template(info)
check_html(html)
Expand Down Expand Up @@ -196,31 +197,52 @@ def test_view_surf(rng):
bg_map=mesh[0][::2, 0])


def test_view_img_on_surf():
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):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

html = view_img_on_surf(mni152_template_res_2, threshold='92.3%')
check_html(html)
html = html_surface.view_img_on_surf(img, threshold=0, surf_mesh=surfaces)

surfaces = datasets.fetch_surf_fsaverage()
html = view_img_on_surf(mni152_template_res_2,
threshold=0,
surf_mesh=surfaces)
check_html(html)
html = html_surface.view_img_on_surf(img, threshold=.4, title="SOME_TITLE")

html = view_img_on_surf(mni152_template_res_2,
threshold=.4,
title="SOME_TITLE")
assert "SOME_TITLE" in html.html
check_html(html)
html = html_surface.view_img_on_surf(
img, threshold=.4, cmap='hot', black_bg=True)

html = view_img_on_surf(
mni152_template_res_2, threshold=.4, cmap='hot', black_bg=True)
check_html(html)
with pytest.raises(DimensionError):
html_surface.view_img_on_surf([img, img])
img_4d = image.new_img_like(img, get_data(img)[:, :, :, np.newaxis])

img_4d = image.new_img_like(
mni152_template_res_2,
get_data(mni152_template_res_2)[:, :, :, np.newaxis])
assert len(img_4d.shape) == 4
html = html_surface.view_img_on_surf(img, threshold='92.3%')
check_html(html)
np.clip(get_data(img), 0, None, out=get_data(img))
html = html_surface.view_img_on_surf(img, symmetric_cmap=False)

np.clip(get_data(mni152_template_res_2),
0,
None,
out=get_data(mni152_template_res_2))
html = view_img_on_surf(mni152_template_res_2, symmetric_cmap=False)
check_html(html)
html = html_surface.view_img_on_surf(img, symmetric_cmap=False,
vol_to_surf_kwargs={
"n_samples": 1,
"radius": 0.,
"interpolation": "nearest"})

html = view_img_on_surf(mni152_template_res_2,
symmetric_cmap=False,
vol_to_surf_kwargs={
"n_samples": 1,
"radius": 0.,
"interpolation": "nearest"})
check_html(html)


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))
Comment on lines +241 to +243
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check that files are accepted as string or paths



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