Allow not scaling background maps for surface plots

[alexisthual]

Closes #3169 .

Currently, this PR:

• enables bg_on_data for plotly engine
• implements scale_bg_map for matplotlib and plotly engines

I still have to:

• enable bg_on_data and scale_bg_map for view_surf()
• add an example (and / or edit existing ones) using this feature

[github-actions]

👋 @alexisthual 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"
• Code is PEP8-compliant.
• (Bug fixes) There is at least one test that would fail under the original bug conditions.
• (New features) There is at least one unit test per new function / class.
• (New features) The new feature is demoed in at least one relevant example.

We will review it as quick as possible, feel free to ping us with questions if needed.

[codecov]

Codecov Report

Merging #3173 (a40ee06) into main (f6db5d4) will increase coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head a40ee06 differs from pull request most recent head 7157971. Consider uploading reports for the commit 7157971 to get more accurate results

@@            Coverage Diff             @@
##             main    #3173      +/-   ##
==========================================
+ Coverage   90.93%   91.02%   +0.09%     
==========================================
  Files         133      133              
  Lines       15386    15414      +28     
  Branches     3212     3219       +7     
==========================================
+ Hits        13991    14031      +40     
+ Misses        832      819      -13     
- Partials      563      564       +1     

Impacted Files	Coverage Δ
nilearn/_utils/docs.py	92.12% <ø> (ø)
nilearn/plotting/html_surface.py	100.00% <100.00%> (ø)
nilearn/plotting/surf_plotting.py	96.28% <100.00%> (+0.12%)	⬆️
nilearn/decoding/space_net.py	88.18% <0.00%> (-0.04%)	⬇️
nilearn/decoding/space_net_solvers.py	98.05% <0.00%> (-0.02%)	⬇️
nilearn/decoding/fista.py	87.65% <0.00%> (ø)
nilearn/decoding/__init__.py	100.00% <0.00%> (ø)
nilearn/decomposition/_base.py	94.87% <0.00%> (ø)
nilearn/decoding/searchlight.py	93.75% <0.00%> (ø)
nilearn/decomposition/canica.py	91.66% <0.00%> (ø)
... and 10 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[alexisthual]

Here are a few snippets working with the current implementation:

Base snippet to run before the others:

import matplotlib.pyplot as plt
from nilearn import datasets, plotting, surface
import numpy as np

from importlib import reload

# %% Load fsaverage5
fs5 = datasets.fetch_surf_fsaverage()
fs7 = datasets.fetch_surf_fsaverage(mesh="fsaverage7")

# %% Load sulc surface from fsaverage
sulc_fs5 = surface.load_surf_data(fs5.sulc_left)
sulc_fs7 = surface.load_surf_data(fs7.sulc_left)
# Create binary map indicating sulci and gyri
sulc_sign_fs5 = (np.sign(sulc_fs5) + 1) / 8 + 0.25
sulc_sign_fs7 = (np.sign(sulc_fs7) + 1) / 8 + 0.25
sulc_norm_fs5 = sulc_fs5 - sulc_fs5.min()
sulc_norm_fs5 = sulc_fs5 / sulc_fs5.max()
sulc_norm_fs7 = sulc_fs7 - sulc_fs7.min()
sulc_norm_fs7 = sulc_fs7 / sulc_fs7.max()

# %% Load random contrast map
motor_images = datasets.fetch_neurovault_motor_task()
surf_fs5 = surface.vol_to_surf(motor_images.images[0], fs5.pial_left)
surf_fs7 = surface.vol_to_surf(motor_images.images[0], fs7.pial_left)

bg_on_data with plotly

# %%
fig = plotting.plot_surf(
    fs5.infl_left,
    surf_fs5,
    cmap="coolwarm",
    threshold=1.5,
    bg_map=sulc_sign_fs5,
    bg_on_data=True,
    scale_bg_map=False,
    engine="plotly",
)
_ = fig.show(renderer="nteract")

# %%
fig = plotting.plot_surf(
    fs7.infl_left,
    surf_fs7,
    cmap="coolwarm",
    threshold=1.5,
    bg_map=sulc_sign_fs7,
    bg_on_data=True,
    scale_bg_map=False,
    engine="plotly",
)
_ = fig.show(renderer="nteract")

Custom background map

# %% Current default usage for bg_map
plotting.plot_surf(
    fs5.infl_left,
    surf_fs5,
    cmap="coolwarm",
    threshold=1.5,
    bg_map=sulc_fs5,
    bg_on_data=True,
    scale_bg_map=True,
)
plt.show()

# %% Binary tones for sulci and gyri
plotting.plot_surf(
    fs5.infl_left,
    surf_fs5,
    cmap="coolwarm",
    threshold=1.5,
    bg_map=sulc_sign_fs5,
    bg_on_data=True,
    scale_bg_map=False,
)
plt.show()

Combining multiple background maps (useful for matplotlib)

Lightning is uniform on 3D figures generated with matplotlib, so it can be hard to make sense of the previous plot.
One can combine background maps like so:

# %% Binary tones for sulci and gyri
plotting.plot_surf(
    fs5.infl_left,
    surf_fs5,
    cmap="coolwarm",
    threshold=1.5,
    bg_map=sulc_sign_fs5 + (sulc_norm_fs5 / 4),
    bg_on_data=True,
    scale_bg_map=False,
)
plt.show()

[jeromedockes]

Thanks a lot @alexisthual I can't do a full review this week but I think the new plots are really cool. Actually I think we should make it easy to use the binary backgrounds that you show (sulc_sign*), either by defining sentinel values for bg_map or by adding a keyword like binarize_bg. we may even want to make it the default.
+1 for adding it to view_surf because we want to keep the interfaces consistent as much as possible

[bthirion]

Thx for opening this !

[alexisthual]

Thanks a lot @alexisthual I can't do a full review this week but I think the new plots are really cool.
Actually I think we should make it easy to use the binary backgrounds that you show (sulc_sign*), either by defining sentinel values for bg_map or by adding a keyword like binarize_bg. we may even want to make it the default.

No worries, this doesn't have to be merged soon.
Moreover, you are right, I think it's important we get the API as simple as possible here.

scale_bg_map clearly is not be the best name. Besides, maybe it would make sense to have a dedicated sulc_map attribute to implement what you suggest, and a sulc_map_mode (name is terrible but please bear with me) instead of scale_bg_map which could be "linear", "sign", "both", "custom" to generate all previous rendering effects.

Overall, it feels to me that bg_map was originally introduced to cope with flat lightning issues coming from matplotlib, but we could find a different way to deal with this problem?
Lastly, even though I now understand it's use, shouldn't we set bg_on_data to True be default? I find plots not using it confusing.

+1 for adding it to view_surf because we want to keep the interfaces consistent as much as possible

I'll do my best 😊

[alexisthual]

Here is a new plot which make me think that it makes more sense to keep bg_map and not add sulc_map, contrary to what I had previously suggested.

Indeed, after experimenting a bit, I think people use the curvature sign (and not the sulc sign), which yields a much nicer rendering:

plotting.plot_surf(
    fs7.infl_left,
    surf_fs7,
    cmap="coolwarm",
    threshold=1.5,
    bg_map=curv_sign_fs7 + sulc_norm_fs7 / 8,
    bg_on_data=True,
    scale_bg_map=False,
)
plt.show()

On a different note, it also works well for flat maps (see #3171 for more info):

flat_fs7 = "/home/alexis/singbrain/fsaverage_flat/surfaces/flat_lh.gii"

# %%
fig = plotting.plot_surf(
    flat_fs7,
    surf_fs7,
    cmap="coolwarm",
    threshold=1.5,
    bg_map=curv_sign_fs7,
    bg_on_data=True,
    scale_bg_map=False,
    view="dorsal",
    engine="plotly"
)
_ = fig.show(renderer="nteract")

[htwangtw]

Since the darkness parameter is applied to background data, should we revise it to bg_darkness or something along the line?

[jeromedockes]

thanks @alexisthual it looks good! as usual with plotting testing is tricky...

[bthirion]

Thx. One general comment is that we should demo these parameters to help users figure out.

[alexisthual]

In the last commits, I tried to make sure that all surface plotting methods (including view_surf and view_img_on_surf) have bg_on_data, scale_bg_map and darkness.

I also edited what I reckon is the main nilearn example for plotting surfaces, so that it shows how to use scale_bg_map=False. I modified the first generated image on purpose, so that it will eventually modify the thumbnail of this example, in order for this feature to be easier to find.
Maybe we could use fsaverage7 in the first image so that it looks nicer. For now, it looks like this on fsaverage5 (with matplotlib and plotly respectively):

Other than that, I think this PR is ready for review 🙂

[NicolasGensollen]

Thanks @alexisthual ! 👍
I had a quick look. I made some small suggestions, but LGTM overall.

[ymzayek]

@alexisthual thanks for the updates and opening the follow-up issues! Seems ready for another round of reviews. Or is there still any TODO's left on your side?

[alexisthual]

I don't think there is any TODO left 😊
I think we could take an hour together (poke @jeromedockes) and check that all plotting functions work as expected, and then we're good to go hopefully!

[ymzayek]

Perhaps this can be the topic for next week's office hour

[bthirion]

I only found a minor glitch. It can be merged otherwise I think.

[jeromedockes]

IIRC we agreed orally during one of the office hours that there would be no such parameter for now, or that it could only be a string corresponding to one of the fsaverage maps?

[jeromedockes]

same here

[jeromedockes]

I chose to include these warnings in this PR since they were pretty simple to add.

The following warning is printed when the code snippet below is ran:

FutureWarning: Background maps are automatically scaled to have values in [0, 1]. This behaviour will be removed in the 0.11 release. You can already simulate this future behaviour by setting bg_map_rescale=False in this function and scaling background maps yourself beforehand.

# %%
import matplotlib.pyplot as plt
from nilearn import datasets, plotting, surface

motor_images = datasets.fetch_neurovault_motor_task()
img = motor_images.images[0]

fsaverage = datasets.fetch_surf_fsaverage()
surface_map = surface.vol_to_surf(img, fsaverage.pial_left)

# %%
plotting.plot_surf(
    fsaverage.pial_left,
    surface_map,
    bg_map=fsaverage.sulc_left,
    threshold=3
)
plt.show()

I'd rather not have a warning show up for plots that use the default values, especially as the message is rather cryptic. in any case we want the behavior / plots to remain correct for someone who uses the fsaverage sulcal depth with default parameters, so I don't think we need to scare users with a warning

[alexisthual]

I'd rather not have a warning show up for plots that use the default values, especially as the message is rather cryptic. in any case we want the behavior / plots to remain correct for someone who uses the fsaverage sulcal depth with default parameters, so I don't think we need to scare users with a warning

I get your concern but I would advocate we need this warning: indeed, it's a deprecation warning (maybe the warning message is not clear) stating that the current default parameters will change in the future.
Maybe we can discuss this today during the office hours!

[jeromedockes]

Maybe we can discuss this today during the office hours!

good idea, see you then!

[alexisthual]

During today's office hours, we discussed:

• removing the new warning which appears when users plot surface information
• removing bg_map_rescale and simply apply the bg_map_rescale="auto" behaviour (ie scale background maps between [0, 1] iif some values are outside [0, 1])
• getting rid of bg_maps for view_img_on_surf() and plot_img_on_surf()

I implemented all 3 in the last commits, so the PR should be good to go!

[jeromedockes]

I think it should be good to go! could you trigger a full build of the documentation to make sure all examples look good? and as we mentioned we may want to visually check a few example pots -- eg the plot with default parameters for each function, or re-running some of the scripts in this PR's discussion, but otherwise I think it should be ready!

thanks for all this work :)
once we add some better background maps in the fsaverage bundles all the nilearn surface plots will be much improved with this PR

[bthirion]

LGTM.

[jeromedockes]

we just went through a bunch of examples prepared by @alexisthual with @ymzayek and everything looks good! thanks a bunch @alexisthual this is a big one :) ! 💯

[bthirion]

Congrats !