mne-tools / mne-connectivity

Connectivity algorithms that leverage the MNE-Python API.
https://mne.tools/mne-connectivity/dev/index.html
BSD 3-Clause "New" or "Revised" License
68 stars 34 forks source link

Plotting connectivity matrices with both negative and positive values should change default colorbar #248

Open JohannesWiesner opened 6 days ago

JohannesWiesner commented 6 days ago

If your issue is a usage question, please consider asking on the MNE Forum instead of opening an issue.

Describe the problem

The default colormap for mne-connectivity.viz.plot_connectivity_circle is "hot". This colorscale only makes sense for positive values. However, my matrix contains both positive and negative values. In this case the colormap should change to a diverging map with two different colors mapping to positive and negative values (e.g. blue to red with black in the middle). 0s should correspond to the background color.

Describe your solution

I currently do this:

    # If the matrix contains both negative and positive values then 
    # the colorscale has to have two different colors with 0 corresponding
    # to the background color
    facecolor = 'white'
    textcolor = 'black'

    if np.any(matrix_values > 0) & np.any(matrix_values < 0):

        # Find the minimum and maximum values in the data and compute
        # midpoint between vmin and vmax where 0 should lie
        vmin = matrix_values.min()
        vmax = matrix_values.max()
        midpoint = abs(vmin) / (vmax - vmin)

        colors = [
            (0, "blue"),  # Negative values: blue
            (midpoint, facecolor),  # Zero: black
            (1, "red")  # Positive values: red
        ]

        # Create the colormap
        cmap = LinearSegmentedColormap.from_list("custom_cmap",colors)

    else:
        cmap = 'hot'
tsbinns commented 5 days ago

There is precedent with things like plot_evoked_topomap in the main package for having the default colourmap adjust to the values:

If None, 'Reds' is used for data that is either all-positive or all-negative, and 'RdBu_r' is used otherwise.

How do people feel about adding similar behaviour to plot_connectivity_circle?

But @JohannesWiesner, could the code you show not be replaced with just passing "RdBu_r" to the colormap parameter?

larsoner commented 5 days ago

How do people feel about adding similar behaviour to plot_connectivity_circle?

Sounds good to me!

tsbinns commented 5 days ago

@JohannesWiesner Would you be able to open a PR for this?

JohannesWiesner commented 1 day ago

But @JohannesWiesner, could the code you show not be replaced with just passing "RdBu_r" to the colormap parameter?

@tsbinns : This has two problems.

1.) I think it would be good if the value 0 always is the same as the "facecolor" argument, so that connections with the value 0 blend in with the background. Therefore the function has to know what the background color is and then build a colorscale taking this information into account

2.) If you're distribution is skewed, setting "RdBu_r" will not result in a colorscale that matches 0 to the background color.

See (this is what happens if you only set "RdBu_r"):

image
tsbinns commented 1 day ago

1.) I think it would be good if the value 0 always is the same as the "facecolor" argument, so that connections with the value 0 blend in with the background.

I would say this is a matter of personal preference, and possibly something already accomodated for by exposing the colormap parameter.

@larsoner already agreed that if data has both negative and positive values, the "RdBu_r" colourmap could be used by default. However, should it also be the case that a custom map is used where those 'zero' values match the background colour, rather than just being white? Or, do we leave this to the user to specify as their own custom colourmap?


2.) If you're distribution is skewed, setting "RdBu_r" will not result in a colorscale that matches 0 to the background color.

Ah, I thought there was some logic in place to account for this. If negative and positive values are present, I also think it makes sense that the 'neutral' colour (i.e., the middle of the diverging colormap) is centered around zero. This could already be done by specifying vmin and vmax, but it could be a nice quality-of-life feature to also do this automatically. What do others think?

JohannesWiesner commented 1 day ago

@tsbinns : Asked this question on Stackoverflow, maybe there is a generic way to customize a default colormap instead of building one from scratch:

https://stackoverflow.com/questions/79134147/customize-a-default-diverging-matplotlib-colormap-so-that-0-always-matches-the-n

JohannesWiesner commented 1 day ago

My personal preference is that 0s are always represented with the background color so that as soon as I export the figure with a transparent color, these connections will not be shown. But of course that might be personal preference (for some users, the value 0 might contain information that should be visible). Notice however, that just choosing "RdBu_r" as default interferes with the facecolor argument. For example if the user sets the facecolor to white it gives you the illusion that some connections don't exist:

image

when you set the facecolor to "black" suddenly these connections pop up:

image

However the latter figure is not very useful in a manuscript because you want the node names to be printed in black so they show up

tsbinns commented 1 day ago

I definitely get that depending on facecolor the legibility of the colourmap changes, e.g., with the hot cmap those very strong values could become difficult to see with a white background: image

The question is whether there should be logic within the plot_connectivity_circle function to handle the interaction between facecolor and colourmap, or whether this is the responsibility of the user by using the available parameters.

I do not have the authority to decide on this though.

tsbinns commented 1 day ago

Asked this question on Stackoverflow, maybe there is a generic way to customize a default colormap instead of building one from scratch

Cool, would be interested to see.

I came across one convenient way, but I don't know how generalisable this is across different ways of creating the colourbar.

In any case for the connectivity visualisation with divergent colourbars, some logic to center it around zero would be a nice feature.

JohannesWiesner commented 1 day ago

Maybe it would also make sense to open another issue? Wouldn't it make more sense to set the background color by default to white and the text color by default to black? Because this is closer to what users would also copy and paste into their text editors / poster editors? From there on, it would make more sense to think about good default colormaps for positive-only and positive-and-negative connectivity matrices?

JohannesWiesner commented 1 day ago

The question is whether there should be logic within the plot_connectivity_circle function to handle the interaction between facecolor and colourmap, or whether this is the responsibility of the user by using the available parameters.

Good point. In case of the latter, it might probably become necessary that the function can accept a norm argument and passes this on to the internal functions.

drammock commented 1 day ago

I agree with https://github.com/mne-tools/mne-connectivity/issues/248#issuecomment-2435902653 that auto-choosing Reds vs RdBu_r is fine, and we can/should leverage what we already have:

https://github.com/mne-tools/mne-python/blob/e15292fc0bc8d5e32dd6d6099a839bf810963f3a/mne/viz/utils.py#L1422-L1430

(which I guess means we should make that function public, if we're going to use it here).

I think it would be too strict to always force the center of a two-slope colormap to be the same as the facecolor. Seems like a nice clear docs example is the best solution, maybe also a note in the Notes section of the docstring. Here's a good starting point:

https://github.com/mne-tools/mne-python/blob/e15292fc0bc8d5e32dd6d6099a839bf810963f3a/examples/time_frequency/time_frequency_erds.py#L79-L90

tsbinns commented 13 hours ago

I think it would be too strict to always force the center of a two-slope colormap to be the same as the facecolor. Seems like a nice clear docs example is the best solution, maybe also a note in the Notes section of the docstring.

Okay, but based off of that example, this means there is also room to add a cnorm parameter to the function?

drammock commented 12 hours ago

yeah sorry I meant to say that explicitly. Adding cnorm seems like the right move here.

tsbinns commented 11 hours ago

So @JohannesWiesner, is this something you are able to create a PR for at this time? Or is there also some help we can offer?