MRG: Add support for a callable in the combine argument of TFR plots #11329
Conversation
mscheltienne
changed the title
|
CI failures seem unrelated. |
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
Just a couple of minor comments, otherwise LGTM!
| if ( | ||
| not isinstance(data, np.ndarray) | ||
| or data.shape != tfr.data.shape[1:] | ||
| ): | ||
| raise RuntimeError( | ||
| "A callable 'combine' must return a numpy array of shape " | ||
| "(n_freqs, n_times)." | ||
| ) |
There was a problem hiding this comment.
| if ( | |
| not isinstance(data, np.ndarray) | |
| or data.shape != tfr.data.shape[1:] | |
| ): | |
| raise RuntimeError( | |
| "A callable 'combine' must return a numpy array of shape " | |
| "(n_freqs, n_times)." | |
| ) | |
| _validate_type(data, np.ndarray, 'data returned by callable') | |
| _check_option('data.shape returned by callable', | |
| data.shape, (tfr.data.shape[1:],)) |
There was a problem hiding this comment.
Honestly, I think the shorter error message with what the shape represent is more informative, even if it takes 4 additional lines (because of the 79 character limit 😄)
There was a problem hiding this comment.
What I wrote should tell you the wrong shape that was supplied and the correct shape that is needed, did you check? I could be wrong...
There was a problem hiding this comment.
Ohh you mean the dimension names. Okay
There was a problem hiding this comment.
Yes, the names. And, IMO, the phrasing from _check_option doesn't really fit this context.
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
|
I didn't notice earlier, but the title generation if set to |
|
|
||
| def _set_title_multiple_electrodes(title, combine, ch_names, max_chans=6, | ||
| all=False, ch_type=None): | ||
| all_=False, ch_type=None): |
There was a problem hiding this comment.
I renamed the argument because of the function all(), and made sure it wasn't used as a kwarg across mne.
LMK if you want me to revert if you think other packages in the ecosystem use this function and might use all= in the function call.
There was a problem hiding this comment.
if they do, their CIs will hopefully tell us :)
|
pip pre failure is unrelated, but I restarted Azure because the Conda job had a 404 that I'd like to see come back green |
|
Pushed a commit that adds a temporary workaround for the mpl issue, marking for merge-when-green, thanks @mscheltienne ! |
|
Okay that wasn't enough... pushed a commit to install |
4 participants
I added support for passing a
callableto control how the channels are combined when plotting a TFR.For now, it only supported
'mean'and'rms'. I could use this feature as I'd like to do a weighted average instead.