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

[BUG] In plot_to_surf vmin is set to 0 despite not setting a threshold #3944

Closed
3 of 9 tasks
eurunuela opened this issue Aug 31, 2023 · 4 comments · Fixed by #3945
Closed
3 of 9 tasks

[BUG] In plot_to_surf vmin is set to 0 despite not setting a threshold #3944

eurunuela opened this issue Aug 31, 2023 · 4 comments · Fixed by #3945
Labels
Bug for bug reports

Comments

@eurunuela
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Operating system

  • Linux
  • Mac
  • Windows

Operating system version

Mac OS Version 13.4.1 "Ventura"

Python version

  • 3.11
  • 3.10
  • 3.9
  • 3.8
  • 3.7

nilearn version

Dev version in #3942

Expected behavior

The minimum value in the colorbar should be set by vmin when the user does not provide a threshold.

Current behavior & error messages

This is what I got:

# Paste the error message here
UserWarning:

choosing both vmin and a threshold is not allowed; setting vmin to 0

I assume threshold has a numerical value by default (rather than None) and that's why vmin is set to 0.

Steps and code to reproduce bug

# Paste your code here
cmap = hm_viridis_onesided
# cmap = "viridis"
vmax = 4
# vmin = vmax/data.shape[1] * 60
vmin = 2
event_count = masker.transform(op.join(out_dir, "data_event_count_avg.nii.gz"))
event_count = np.squeeze(event_count)


texture = vol_to_surf(utils.color_rois(out_dir, mask, event_count), fsaverage.pial_left,interpolation='nearest',radius =1, n_samples=1)
surf_plot1=plotting.plot_surf_roi(fsaverage.infl_left, texture, hemi='left', cmap = cmap, colorbar=True, vmin = vmin, vmax = vmax, bg_map=fsaverage.sulc_left, engine="plotly")
@eurunuela eurunuela added the Bug for bug reports label Aug 31, 2023
@bthirion
Copy link
Member

bthirion commented Sep 1, 2023

Thx for reporting.
This probably a side effect of making vmin explicit, which was not done in the original implementation.

@ymzayek
Copy link
Member

ymzayek commented Sep 1, 2023

It looks like a bug for the plotly engine. Works as it should for matplotlib. Here's a code to more quickly reproduce the warning and buggy behaviorr:

from nilearn import datasets
from nilearn import plotting

destrieux_atlas = datasets.fetch_atlas_surf_destrieux()
parcellation = destrieux_atlas['map_left']

fsaverage = datasets.fetch_surf_fsaverage()
plot_1 = plotting.plot_surf_roi(
            fsaverage.infl_left,
            parcellation,
            hemi="left",
            cmap="viridis_r",
            vmin=2,
            vmax=50,
            colorbar=True,
            bg_map=fsaverage.sulc_left,
            engine="plotly",
         )
plot_1.show()

I'll follow up with a PR to fix

@ymzayek
Copy link
Member

ymzayek commented Sep 1, 2023

Looking more into it, would it be better to allow both vmin and threshold to be passed to mimic matplotlib behavior?

Allowing this would change the behavior of js_plotting_utils.colorscale function where the vmin returned would be changed leading to a change in how the colorbar looks and the data scaling in the plot (to be the same as matplotlib).

As it is, the warning will always be raised unless the user defines threshold=None because by default it is given a float. If we want to keep the current behavior then the warning should say that the user needs to set threshold=None if they want to use vmin. And we'd also need to move a few lines under plot_surf_roi that automatically set vmin if it is not specified by the user.

I think the first option is preferable especially if we consider this a bug and behavior should look like matplotlib.

New behavior WITHOUT warning with vmin set to 10 and threshold set to 20:
new

Old behavior with warning with vmin set to 10 and threshold set to 20:
newplot

With the second option the plot would be the same as the second plot but the warning would change.

@bthirion
Copy link
Member

bthirion commented Sep 1, 2023

Looks good. We should make sure that the behavior is consistent btw mpl and plotly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug for bug reports
Projects
None yet
3 participants