mne-python icon indicating copy to clipboard operation
mne-python copied to clipboard

Remove average parameter from CSP and SPoC plotting methods

Open tsbinns opened this issue 1 year ago • 3 comments

Describe the new feature or enhancement

Discussed in call with @wmvanvliet. Not so much a feature request but a feature un-request...

The CSP and SPoC classes of the decoding module have the plotting methods plot_filters() and plot_patterns() for visualising the filters and patterns of each component as a topomap. Internally, this plotting works by packaging the filters/patterns into an EvokedArray object and calling the plot_topomap() method.

Evoked.plot_topomap() has the average parameter for plotting data averaged over times, which is also supported by the plot_filters/patterns() methods of the CSP and SPoC classes, where here it functions to average over components. This has two issues:

  1. The docstring entry for the decoding classes is the same as in Evoked.plot_topomap, so it refers to averaging over times: "The time window (in seconds) around a given time point to be used for averaging. For example, 0.2 would translate into a time window that starts 0.1 s before and ends 0.1 s after the given time point...".
  2. It is questionable whether there should be the option to average over these independent components. They should really be handled separately and having the average method could give a user not so familiar with the methods the impression that this is an acceptable way to interpret the spatial filters/patterns.

Describe your proposed implementation

Remove the average parameter from the plot_filters() and plot_patterns() methods of mne.decoding.CSP and mne.decoding.SPoC classes.

Describe possible alternatives

If people are against removing this functionality, at the very least could update the docstrings to remove references to averaging over times, and instead to averaging over components which is what the argument is actually doing.

Additional context

No response

tsbinns avatar Jun 14 '24 12:06 tsbinns

This is another example how the docdict does not make docstrings more consistent. I don't have an opinion regarding what to do about the averaging, but at the very least I'd add the correct docstring in plain text (no docdict entry since we'll hopefully be moving away from it).

cbrnr avatar Jun 15 '24 15:06 cbrnr

This is another example how the docdict does not make docstrings more consistent

We've discussed this before elsewhere. The docdict does improve consistency but does not prevent mistakes/inaccuracies (like this one). As you note, we're trying to come up with a better system. Until we do that, can we refrain from requesting that every PR reduce docdict usage? The primary question here is whether to keep the parameter at all, or just change the param description

drammock avatar Jun 15 '24 15:06 drammock

+1 to remove the param entirely.

mscheltienne avatar Jun 23 '24 19:06 mscheltienne

Just wanted to follow up on this. Seems there is support from @mscheltienne and @wmvanvliet to remove the parameter. If others approve and I would open a PR, should it just be a deprecation warning for now?

tsbinns avatar Sep 05 '24 09:09 tsbinns

+1 to remove. Should get a deprecation cycle

drammock avatar Sep 05 '24 14:09 drammock