[ENH] Allow setting `vmin` in `plot_glass_brain` and `plot_stat_map` #3993

michellewang · 2023-09-19T22:21:59Z

Changes proposed in this pull request:

Made plot_glass_brain use its vmin argument instead of silently ignoring it
- This involved changing _get_colorbar_and_data_ranges, which changes the behaviour of plot_stat_map to also allow vmin to be set (instead of throwing an error)
Updated docstring for symmetric_cbar, since the colorbar will no longer be restricted to (-vmax, vmax), (0, vmax), (-vmax, 0), and the colormap range will also change depending on vmin/vmax
Removed symmetric_data_range argument from _get_colorbar_and_data_ranges, which was added in [ENH] add vmin argument to plotting.plot_img_on_surf #3873 to preserve the old behaviour in these functions. We can still get the symmetric data range behaviour if:
- symmetric_cbar is True, or
- symmetric_cbar is 'auto', vmin or vmax is None, and the data has both positive and negative values
  - This means that a call with default arguments will plot a symmetric colorbar, same as before
Refactored test cases for _get_colorbar_and_data_ranges to have clearer expected results
Added some example glass brain plots in examples/01_plotting/plot_demo_glass_brain_extensive.py
- Other glass brain examples still show the same plots

Example plot_glass_brain calls with vmin:

from nilearn import datasets, plotting

stat_img = datasets.load_sample_motor_activation_image()

plotting.plot_glass_brain(
    stat_img,
    colorbar=True,
    display_mode="lyrz",
    threshold=2,
    cmap='viridis',
    vmin=2,                  # use the whole colormap range even if there is a threshold
    symmetric_cbar=False,
)

plotting.plot_glass_brain(
    stat_img,
    colorbar=True,
    display_mode="lyrz",
    threshold=2,
    cmap='viridis',
    vmin=-5,
    vmax=8,
    plot_abs=False,
)

Example plot_stat_map calls with vmin:

from nilearn import datasets, plotting
stat_img = datasets.load_sample_motor_activation_image()

plotting.plot_stat_map(
    stat_img,
    colorbar=True, 
    threshold=3,
    vmin=-9,   # symmetric from -9 to 9
)

plotting.plot_stat_map(
    stat_img,
    colorbar=True, 
    threshold=3,
    vmin=0,             # non-negative values only
    vmax=10,
    symmetric_cbar=False,
    cmap='viridis',
)

plotting.plot_stat_map(
    stat_img,
    colorbar=True, 
    threshold=3,
    vmin=3,            # use the whole colormap range even if there is a threshold
    vmax=10,
    symmetric_cbar=False,
    cmap='viridis',
)

github-actions · 2023-09-19T22:22:13Z

👋 @michellewang 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 · 2023-09-19T22:34:22Z

Codecov Report

Merging #3993 (58f717d) into main (0b54555) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3993      +/-   ##
==========================================
- Coverage   91.65%   91.59%   -0.06%     
==========================================
  Files         145      145              
  Lines       16222    16238      +16     
  Branches     3370     3376       +6     
==========================================
+ Hits        14869    14874       +5     
- Misses        810      817       +7     
- Partials      543      547       +4

Flag	Coverage Δ
macos-latest_3.10	`91.51% <100.00%> (-0.06%)`	⬇️
macos-latest_3.11	`91.51% <100.00%> (-0.06%)`	⬇️
macos-latest_3.12	`91.51% <100.00%> (-0.06%)`	⬇️
macos-latest_3.8	`91.47% <100.00%> (-0.06%)`	⬇️
macos-latest_3.9	`91.48% <100.00%> (-0.06%)`	⬇️
ubuntu-latest_3.10	`91.51% <100.00%> (-0.06%)`	⬇️
ubuntu-latest_3.11	`91.51% <100.00%> (-0.06%)`	⬇️
ubuntu-latest_3.12	`91.51% <100.00%> (-0.06%)`	⬇️
ubuntu-latest_3.8	`91.47% <100.00%> (-0.06%)`	⬇️
ubuntu-latest_3.9	`91.48% <100.00%> (-0.06%)`	⬇️
windows-latest_3.10	`91.46% <100.00%> (-0.06%)`	⬇️
windows-latest_3.11	`91.46% <100.00%> (-0.06%)`	⬇️
windows-latest_3.12	`91.46% <100.00%> (-0.06%)`	⬇️
windows-latest_3.8	`91.42% <100.00%> (-0.06%)`	⬇️
windows-latest_3.9	`91.43% <100.00%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
nilearn/_utils/docs.py	`91.86% <ø> (ø)`
nilearn/plotting/displays/_slicers.py	`90.93% <100.00%> (+0.05%)`	⬆️
nilearn/plotting/img_plotting.py	`88.00% <100.00%> (-1.55%)`	⬇️
nilearn/plotting/surf_plotting.py	`94.30% <ø> (ø)`

... and 1 file with indirect coverage changes

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

ymzayek · 2023-09-20T09:37:36Z

Thx @michellewang for working on this! The example plots look great! Will give a more thorough review soon.

ymzayek

Overall all, looks pretty good! Can you explain the refactoring concerning the plotting of absolute values?

nilearn/_utils/docs.py

nilearn/plotting/displays/_slicers.py

michellewang · 2023-09-26T16:49:58Z

Can you explain the refactoring concerning the plotting of absolute values?

Yes, I had to move the thresholding because it needs to happen after we take the absolute value of the data (which happens in GlassBrainAxes.transform_to_2d). Otherwise, when plot_abs is True, vmin is set to 0, and the new thresholding function will mask all negative values in the data (if a non-zero threshold is defined), so the resulting plot will only show values that were positive in the original data instead of the absolute value of all values.

I'll remove the lines with data_selection = np.abs(data_selection) in nilearn/plotting/displays/_axes.py because after looking at them again I think they are redundant with what the code already did.

michellewang · 2023-09-26T17:00:16Z

Also: are there any tests for slicers and _map_show? I couldn't find any from a quick search

ymzayek · 2023-10-02T11:38:47Z

Also: are there any tests for slicers and _map_show? I couldn't find any from a quick search

Slicers are tested in nilearn/plotting/tests/test_displays.py but _map_show is not tested directly (only through calls of add_contours and add_overlay). I'd suggest adding tests relevant to the changes you made.

bthirion

Looks great overall ! Thx !

nilearn/_utils/docs.py

nilearn/plotting/img_plotting.py

bthirion

LGTM. Can you consider codecov complaints ?

nilearn/plotting/tests/test_img_plotting/test_plot_glass_brain.py

examples/01_plotting/plot_demo_glass_brain_extensive.py

ymzayek · 2023-10-09T14:22:04Z

LGTM. Can you consider codecov complaints ?

Not sure why that is happening. I checked the report and the added code is fully covered and the codecov/patch report is 100% so I think it's ok. Seems like there is a small decrease on the project level but this seems to come from indirect changes. I think @michellewang you can just make the changes from the last review and this should be good to go.

michellewang · 2023-10-09T22:13:55Z

Ok, I was confused by the Codecov report. Just addressed the remaining comments, thanks @ymzayek and @bthirion for reviewing!

ymzayek

Just FYI something went wrong with the merge so I redid it. This PR LGTM, thanks @michellewang!

bthirion

LGTM, thx.

Remi-Gau · 2024-02-02T11:48:26Z

nilearn/plotting/displays/_slicers.py

-        if threshold is not None:
-            data = _safe_get_data(img, ensure_finite=True)
-            if threshold == 0:
-                data = np.ma.masked_equal(data, 0, copy=False)
-            else:
-                data = np.ma.masked_inside(
-                    data, -threshold, threshold, copy=False
-                )
-            img = new_img_like(img, data, img.affine)
-


removing this causes a regression [BUG] plot_roi cannot handle cmap with only 1 level #4255

michellewang added 9 commits September 11, 2023 23:27

add vmin to plot_glass_brain

9b4ba9e

update thresholding

943033a

add test for plot_glass_brain

e26c79e

update _get_colorbar_and_data_ranges() and tests

df374fb

use half of colormap if plot_abs is True

bf597fa

update docstrings

6d121f6

fix plot_abs and nan vmin/vmax

65a518f

minor refactoring

0f49ef7

add example with vmin

7da66f1

michellewang added 4 commits September 19, 2023 23:28

fix flake8 and do thresholding after transform_to_2d

070d724

update example

174366e

attempt to increase test coverage

751d054

Merge branch 'main' into 3084/more_vmin

1c3bb48

michellewang changed the title ~~[ENH] Add vmin argument to plot_glass_brain and plot_stat_map~~ [ENH] Allow setting vmin in plot_glass_brain and plot_stat_map Sep 20, 2023

add changelog entry

35f73d4

ymzayek reviewed Sep 21, 2023

View reviewed changes

nilearn/_utils/docs.py Outdated Show resolved Hide resolved

nilearn/plotting/displays/_slicers.py Outdated Show resolved Hide resolved

michellewang added 3 commits September 26, 2023 11:36

Merge branch 'main' into 3084/more_vmin

8dff80f

move default="auto" for symmetric_cbar to substitution string

d420580

remove redundant absolute value transformation

10c1ff0

michellewang added 2 commits October 2, 2023 09:53

Merge remote-tracking branch 'upstream/main' into 3084/more_vmin

fdfbb9f

attempt to increase test coverage

a082c03

bthirion reviewed Oct 2, 2023

View reviewed changes

nilearn/_utils/docs.py Outdated Show resolved Hide resolved

nilearn/plotting/img_plotting.py Outdated Show resolved Hide resolved

michellewang added 2 commits October 5, 2023 12:28

update docs for symmetric_cbar

3a9b38e

replace error with warning for plot_abs/vmin case

a49efe8

michellewang added 2 commits October 5, 2023 12:33

Merge remote-tracking branch 'upstream/main' into 3084/more_vmin

56ab1f9

allow arbitrary vmin even if plot_abs is True

b04548e

bthirion reviewed Oct 6, 2023

View reviewed changes

ymzayek reviewed Oct 6, 2023

View reviewed changes

ymzayek added 2 commits October 10, 2023 10:24

Apply suggestions from code review

c45e8cd

Merge branch 'main' into HEAD

58f717d

ymzayek force-pushed the 3084/more_vmin branch from 6999ba5 to 58f717d Compare October 10, 2023 08:27

ymzayek approved these changes Oct 10, 2023

View reviewed changes

ymzayek requested a review from bthirion October 10, 2023 12:34

bthirion approved these changes Oct 10, 2023

View reviewed changes

ymzayek merged commit c018480 into nilearn:main Oct 11, 2023
32 checks passed

michellewang deleted the 3084/more_vmin branch November 1, 2023 22:15

michellewang mentioned this pull request Nov 2, 2023

[ENH] Colorbar ticks at threshold values #2887

Merged

Remi-Gau mentioned this pull request Feb 2, 2024

[BUG] plot_roi cannot handle cmap with only 1 level #4255

Closed

9 tasks

Remi-Gau reviewed Feb 2, 2024

View reviewed changes

Remi-Gau mentioned this pull request Feb 2, 2024

[FIX] Fix color bar handling with color map with only 1 level #4256

Merged

handwerkerd mentioned this pull request May 6, 2024

minimum nilearn to 0.10.3 ME-ICA/tedana#1094

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Allow setting `vmin` in `plot_glass_brain` and `plot_stat_map` #3993

[ENH] Allow setting `vmin` in `plot_glass_brain` and `plot_stat_map` #3993

michellewang commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

codecov bot commented Sep 19, 2023 •

edited

ymzayek commented Sep 20, 2023

ymzayek left a comment

michellewang commented Sep 26, 2023

michellewang commented Sep 26, 2023

ymzayek commented Oct 2, 2023 •

edited

bthirion left a comment

bthirion left a comment

ymzayek commented Oct 9, 2023

michellewang commented Oct 9, 2023

ymzayek left a comment

bthirion left a comment

Remi-Gau Feb 2, 2024

[ENH] Allow setting vmin in plot_glass_brain and plot_stat_map #3993

[ENH] Allow setting vmin in plot_glass_brain and plot_stat_map #3993

Conversation

michellewang commented Sep 19, 2023

github-actions bot commented Sep 19, 2023

codecov bot commented Sep 19, 2023 • edited

Codecov Report

ymzayek commented Sep 20, 2023

ymzayek left a comment

Choose a reason for hiding this comment

michellewang commented Sep 26, 2023

michellewang commented Sep 26, 2023

ymzayek commented Oct 2, 2023 • edited

bthirion left a comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

ymzayek commented Oct 9, 2023

michellewang commented Oct 9, 2023

ymzayek left a comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

Remi-Gau Feb 2, 2024

Choose a reason for hiding this comment

[ENH] Allow setting `vmin` in `plot_glass_brain` and `plot_stat_map` #3993

[ENH] Allow setting `vmin` in `plot_glass_brain` and `plot_stat_map` #3993

codecov bot commented Sep 19, 2023 •

edited

ymzayek commented Oct 2, 2023 •

edited