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

[JohannesWiesner]

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]

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]

How do people feel about adding similar behaviour to plot_connectivity_circle?

Sounds good to me!

[tsbinns]

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

[JohannesWiesner]

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"):

[tsbinns]

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]

@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]

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:

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

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]

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:

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]

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]

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]

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]

I agree with #248 (comment) 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]

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]

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

[tsbinns]

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?

[JohannesWiesner]

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?

I don't have time for this at the moment, but maybe over the Christmas holidays.

Just a quick sanity check: if the matrix data is normally distributed, shouldn't a TwoSlopeNorm RdBu_r color scale automatically converge to a “normal” RdBu_r color scale? If so, and if there are future plans to make the automatic selection of color scales for matrices with one or two signs “smarter”, the latter could always be the TwoSlopeNorm of RdBu_r.

On top, I would still pitch for a default white background with black text. In that case, 0 values for both the one-sign / two-sign colorscales should always correspond to the white background color.

[tsbinns]

I don't have time for this at the moment, but maybe over the Christmas holidays.

There's no rush from our end. Unfortunately I would also not have time to work on this before January, but if there's still work to be done then I am happy to try lend a hand!

if the matrix data is normally distributed, shouldn't a TwoSlopeNorm RdBu_r color scale automatically converge to a “normal” RdBu_r color scale?

If by normally distributed you mean centered around zero, I think yes.

If so, and if there are future plans to make the automatic selection of color scales for matrices with one or two signs “smarter”, the latter could always be the TwoSlopeNorm of RdBu_r.

I think the hangup here would be that this represents a change in behaviour that could disrupt people's existing code. Currently if you would pass RdBu_r, there would be no normalisation. Making normalisation the default for positive and negative values might therefore require a deprecation cycle to allow users to adjust to the new behaviour.
As such, it's a bit easier if a new cnorm parameter is added which offers the new functionality without affecting existing behaviour.

I would still pitch for a default white background with black text. In that case, 0 values for both the one-sign / two-sign colorscales should always correspond to the white background color.

So again here, updating default behaviour like this would require a deprecation cycle. Also as mentioned above, forcing zero values to match the background colour is a bit strict. I agree that the ERD example linked to covers this nicely. Something similar could be done in an example here to show how plotting can be customised to personal preferences.