-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
VWIP Topoplot refactor #5568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VWIP Topoplot refactor #5568
Conversation
| type_name = ', '.join(cls.__name__ for cls in iter_types) | ||
| if not isinstance(item, types): | ||
| raise TypeError(item_name, ' must be an instance of ', type_name, | ||
| raise TypeError(item_name + ' must be an instance of ' + type_name + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change? and either way once we release we would be able to start using fstrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it printed weird before.
Don't see how an fstring would be better here. You could already do it with string interpolation right? this is very transparent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @larsoner fixed the very same bug in your recent PR anyways :) via string interpolation
|
|
||
|
|
||
| def _plot_evoked_topo(evoked, layout=None, layout_scale=0.945, color=None, | ||
| def _handle_grads_for_topos(evokeds, noise_cov=None, scalings=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we come up with a better name than handle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm not sure how to call what it's doing. (Note I didn't write that code, only moved it.)
|
Ok the current implementation is bad. Will hopefully solve it better with @drammock :) |
|
Thanks for looking over it though @massich |
Have plot_evoked_topo use plot_compare_evokeds.
Benefits: looks nicer, CIs, code reuse, API consistency.
preemptively tagging @sappelhoff @mmagnuski @cbrnr @choldgraf ...
closes #4746