mne-tools / mne-nirs

Process Near-Infrared Spectroscopy Data in MNE
https://mne.tools/mne-nirs/
BSD 3-Clause "New" or "Revised" License
79 stars 35 forks source link

vlim boundaries not working properly for plot_glm_group_topo #522

Closed HanBnrd closed 4 days ago

HanBnrd commented 12 months ago

Describe the bug

When None is used as one of the boundaries for vlim in plot_glm_group_topo, it is not actually taking the min/max of the data.

Steps to reproduce

According to those lines of code, here are, I think, some problematic cases:

Expected results

The function actually uses the min or max of the data for the vlim boundary set to None.

Suggested fix

Replacing this by this:

    vmin, vmax = vlim
if vmax is None:
    vmax = np.max(estimates)
if vmin is None:
    vmin = np.min(estimates)
larsoner commented 12 months ago

Can you check the default cmap? If it's RdBu_r for example the idea is that the white midpoint should stay at zero. So if you set the max the min should be -max, and if you set the min the max should be -min.

I think ideally we would maintain compatibility with however we handle this stuff in MNE-Python topomap plotting. It could be the above behavior but that needs to be checked...

HanBnrd commented 12 months ago

Yes I should have specified, the default is RdBu_r indeed, however I was think about it more in the the case where we would use mpl.cm.Oranges and mpl.cm.Blues_r as in this example:

# Plot the two conditions
plot_glm_group_topo(raw_haemo.copy().pick(picks="hbo"),
                    ch_model_df.query("Condition in ['Tapping_Left']"),
                    colorbar=False, axes=axes[0, 0],
                    vlim=(0, 20), cmap=mpl.cm.Oranges)

plot_glm_group_topo(raw_haemo.copy().pick(picks="hbo"),
                    ch_model_df.query("Condition in ['Tapping_Right']"),
                    colorbar=True, axes=axes[0, 1],
                    vlim=(0, 20), cmap=mpl.cm.Oranges)

# Plot the two conditions
plot_glm_group_topo(raw_haemo.copy().pick(picks="hbr"),
                    ch_model_df.query("Condition in ['Tapping_Left']"),
                    colorbar=False, axes=axes[1, 0],
                    vlim=(-10, 0), cmap=mpl.cm.Blues_r)
plot_glm_group_topo(raw_haemo.copy().pick(picks="hbr"),
                    ch_model_df.query("Condition in ['Tapping_Right']"),
                    colorbar=True, axes=axes[1, 1],
                    vlim=(-10, 0), cmap=mpl.cm.Blues_r)

I feel that it would make sense to possibly use for HbO data vlim=(0, None) where we would expect the vmax to be the highest positive value, and for HbR data vlim=(None, 0) where we would expect the vmin to be the lowest negative value.

But I understand how the current implementation makes sense in the RdBu_r case. However I feel like it's more common to use shades of red/orange for HbO (the more positive the darker) and shades of blue for HbR (the more negative the darker).

larsoner commented 12 months ago

Maybe to support both use cases instead of changing what None means or how it behaves we could add a new behavior -- make it so you could do ("min", 0) and (0, "max")

HanBnrd commented 12 months ago

Actually, thinking more about it, and after checking NIRS-toolbox, I feel like RdBu_r is probably the best way to use this function, because forcing colours for HbO and HbR could look misleading.

Here is a NIRS-toolbox example using the same red to blue for both HbO and HbR:

image (NIRS-toolbox topo)

I'm not sure if the tutorial should show how to do different colours for each chroma... Though it's good to showcase capabilities.

larsoner commented 11 months ago

Either way is fine with me... maybe a .. note:: somewhere about how to be careful with vlim would make sense

HanBnrd commented 4 days ago

Closing because the behaviour makes sense