should plot_surf_roi raise when provided invalid data (ie not positive integers)?

[eurunuela]

Is there an existing issue for this?

• [X] I have searched the existing issues

Operating system

• [ ] Linux
• [X] Mac
• [ ] Windows

Operating system version

For example one of the following:

• Linux Ubuntu 22.04
• Mac OS Version 12 "monterey"
• Windows 11

Python version

• [ ] 3.12
• [ ] 3.11
• [ ] 3.10
• [ ] 3.9
• [X] 3.8

nilearn version

0.10.3.dev40+g0fa9ba3c (from main) as of 07:56:43 UTC October 26th, 2023.

Expected behavior

The map should have negative values. vmax was set to 0.3, and vmin to -0.3.

Current behavior & error messages

This is what I got:

Steps and code to reproduce bug

def plot_hemisphere(texture, contours, hemisphere, cmap, vmin, vmax, view="lateral"):
    """texture is generated by vol_to_surf"""

    fig, ax = plt.subplots(nrows=1, ncols=1, figsize=(6, 6), subplot_kw={"projection": "3d"})
    fig.tight_layout(h_pad=None, w_pad=None)

    if hemisphere == "left":
        mesh = fsaverage.infl_left
        bg_map = fsaverage.sulc_left
    else:
        mesh = fsaverage.infl_right
        bg_map = fsaverage.sulc_right

    surf_plot = plot_surf_roi(
        mesh,
        texture,
        hemi=hemisphere,
        cmap=cmap,
        axes=ax,
        colorbar=True,
        vmin=vmin,
        vmax=vmax,
        bg_map=bg_map,
        darkness=0.4,
        view=view,
    )

    # # Check levels based on contours and adjust accordingly. Just it set up for 2 levels for now
    # if 2 in contours:
    #     levels = [1, 2]
    #     colors = ["black", "blue"]
    # else:
    #     levels = [1]
    #     colors = ["black"]

    plot_surf_contours(
        mesh,
        contours,
        figure=surf_plot,
        legend=False,
        axes=ax,
        levels=[1],
        colors=["black"],
        linewidth=5,
        view=view,
    )

hemispheres = ["left", "right"]

for hemisphere in hemispheres:
# Convert both textures only once
texture = vol_to_surf(
    isrsa_mantel_r2_brain,
    getattr(fsaverage, f"pial_{hemisphere}"),
    interpolation="nearest",
    radius=1,
    n_samples=1,
)
contours = vol_to_surf(
    roi_list,
    getattr(fsaverage, f"pial_{hemisphere}"),
    interpolation="nearest",
    radius=1,
    n_samples=1,
)

# Lateral views
plot_hemisphere(texture, contours, hemisphere, cmap, vmin, vmax)

# Medial views
plot_hemisphere(texture, contours, hemisphere, cmap, vmin, vmax, view="medial")

[Remi-Gau]

Just to check if this is a regression, can you quickly test with less recent versions of Nilearn if you have the same problem?

[eurunuela]

Just to check if this is a regression, can you quickly test with less recent versions of Nilearn if you have the same problem?

I have tried with 0.8, 0.10 and the dev version 😅

[Remi-Gau]

OK defo not a regression then. Or at least not a recent one.

[eurunuela]

I wonder if it is related to #3944 in any way.

[Remi-Gau]

Possibly. Will try to write a tiny regression test for it.

[eurunuela]

I can send you my file via Mattermost if you want @Remi-Gau!

[Remi-Gau]

na that's fine the test will have to be added to the test suite so it should only rely on data we can easily generate on the fly

[Remi-Gau]

Just to check: if you format the tick of the color bar to show more decimals does it make more sense?

See below

import numpy as np

from nilearn import plotting
from nilearn.conftest import _rng
from nilearn.surface.tests._testing import generate_surf

def _generate_data_test_surf_roi():
    """generate mash, roi map filled with 1 and -1 values, and parcellation"""
    mesh = generate_surf()
    roi_map = np.ones(mesh[0].shape[0])
    roi_idx = _rng().randint(0, mesh[0].shape[0], size=10)
    roi_map[roi_idx] = -1
    parcellation = _rng().uniform(size=mesh[0].shape[0])
    return mesh, roi_map, parcellation

mesh, roi_map, parcellation = _generate_data_test_surf_roi()
plotting.plot_surf_roi(
    mesh,
    roi_map=roi_map,
    engine="matplotlib",
    cmap="coolwarm",
    colorbar=True,
    vmin=-1.5,
    vmax=1,
    view="lateral",
    cbar_tick_format="%.2g", # to make sure the values of ticks of the cbar are properly dsiplayed
)

plotting.show()

[Remi-Gau]

@ymzayek am realizing that in many cases the dummy data we use to test the plotting functions do not include any negative values.

this may be worth checking that our tests can handle those as well, no?

[ymzayek]

@Remi-Gau hm ok yes we should definitely check that. Are you referring to specific fixtures or internal testing data generators or all of it?

Concerning this issue, as pointed out in https://github.com/nilearn/nilearn/issues/4082#issuecomment-1780916405 seems to me like a formatting issue and not a bug. @eurunuela can you add cbar_tick_format="%.2g" parameter to plot_surf_roi in your code and let us know what happens?

[eurunuela]

Concerning this issue, as pointed out in https://github.com/nilearn/nilearn/issues/4082#issuecomment-1780916405 seems to me like a formatting issue and not a bug. @eurunuela can you add cbar_tick_format="%.2g" parameter to plot_surf_roi in your code and let us know what happens?

Yes! That fixes the issue. It's quite odd though that the colorbar would show a minimum of 0. Thank you so much @Remi-Gau & @ymzayek!

Edit: But since the values are below 1, I guess it makes sense it only showed the 0.

[Remi-Gau]

Are you referring to specific fixtures or internal testing data generators or all of it?

Not checked them all but in nilearn/plotting/tests/test_surf_plotting.py

This one is function that genrates only 0 and 1: https://github.com/nilearn/nilearn/blob/5d32855dc6af30aad0aa3b20d6a650dddff1231f/nilearn/plotting/tests/test_surf_plotting.py#L705C1-L711C39

The img_3d_mni fixture only contains positive values: https://github.com/nilearn/nilearn/blob/5d32855dc6af30aad0aa3b20d6a650dddff1231f/nilearn/plotting/tests/test_surf_plotting.py#L817C1-L832C1

[eurunuela]

Yeah, it seems like you probably want to add a negative decimals example to your tests.

[ymzayek]

Yes we should have some second versions of those minimal fixtures with negative values. @Remi-Gau I suggest to open an issue detailing this and we can close this one if @eurunuela your reported issue is resolved.

[eurunuela]

My issue is resolved. Thank you!

I will let @Remi-Gau close this when he opens the issue for the tests.

[jeromedockes]

plot_surf_roi is for showing atlas region indices, so it is meant to deal with positive integers only. we should probably raise an error when negative or floating-point values are passed to this function

[jeromedockes]

from the documentation

The value at each vertex one inside the ROI and zero inside ROI, or an integer giving the label number for atlases.

[ymzayek]

I'll reopen this then. It makes sense to raise an error but I'm wondering about the use case of @eurunuela

[jeromedockes]

I'd rather we open another one because the existing discussion about the colorbar distracts from the actual issue

[ymzayek]

Sure! I was misled by the name change. Closing again and will let you open it :)