Extend plot_csd support to SEEG, ECoG, and DBS channel types#13713
Extend plot_csd support to SEEG, ECoG, and DBS channel types#13713Aniketsy wants to merge 13 commits intomne-tools:mainfrom
Conversation
tsbinns
left a comment
There was a problem hiding this comment.
Thanks for opening this, definitely on the right track! Some comments below.
|
@tsbinns thanks for the review and for suggesting this approach, it makes much more sense than my earlier implementation. please have a look and let me know if any further improvements are needed. |
tsbinns
left a comment
There was a problem hiding this comment.
Good changes! A couple small comments on the code, and a slightly bigger one for to refine the test. Let me know if anything's unclear.
|
@larsoner @wmvanvliet I noticed that the existing behaviour when plotting is that if no valid channel types are present (currently only EEG, Mag, Grad; this PR would change it to all valid data types), it just returns an empty I was a bit surprised by this. My expectation was at least a warning would be given that there is nothing valid to plot. WDYT? |
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
tsbinns
left a comment
There was a problem hiding this comment.
Good progress! A couple minor comments to tidy things up.
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
|
@Aniketsy I'd say this is looking good, but I'd still like to get this comment addressed (#13713 (comment)) and maybe also a review from @wmvanvliet. Please also add your name to |
I noticed this ci failure in two PR's @tsbinns should we relax the statsmodel version requirement? |
|
That was addressed in #13728. Don't worry about merging the updated branch until there's new code to push. Will save us some cost on running CI jobs. |
|
Just saw it. Makes sense |
Fixes #13711