MRG Ica plot - #2251
Conversation
|
I was going to suggest that you do some heavy refactoring to unify bits of
code but it sounds like you're already on it! Excellent.
|
There was a problem hiding this comment.
we should avoid mutable defaults e.g. lists.
|
cool let us know when it's ready. If not yet. |
|
awesome ! how should I test this? Is there an example I can run? |
|
I added a line in the example that uses this function. If you're gonna run it many times, it's probably wise to save the ica solution and use that. |
|
I made it so that the plotting function now returns the updated list of components marked for exclusion, as selected by the user. Would it be okay to exclude the components straight away on closing the figure (ica.exclude)? At the moment it only updates the list, but is inconsistent with other plot functions because it returns a tuple of figure and list instead of just the figure. |
There was a problem hiding this comment.
did we already discuss why not simply updating ICA.exclude?
There was a problem hiding this comment.
No
On 2 juil. 2015, at 16:06, Denis A. Engemann notifications@github.com wrote:
In mne/preprocessing/ica.py:
Defaults to (1., 0., 0.) (red).show : boolShow figures if True. Defaults to True.block : boolWhether to halt program execution until the figure is closed.Useful for selecting components for exclusion on the fly(click on line). May not work on all systems / platforms.Defaults to False.
Returns
fig : Instance of matplotlib.figure.FigureThe figure.exclude : listUpdated list of components marked for exclusion."""—
Reply to this email directly or view it on GitHub.
I would opt for that. |
|
+1
|
There was a problem hiding this comment.
This example is quite crazy. It opens 12 or 13 figures ! anything can be done about it? cc @dengemann
There was a problem hiding this comment.
I would like to leave it like this, it has been very useful for tracking bugs in the past.
|
There is no help button here. I suppose you'll add that soon? |
|
The help button positioning is partly dependent on the layout remake at #2214. I'd rather wait for it to be merged before implementing it here. |
|
@jaeilepp @mainakjas @agramfort I still think we should just overwrite plot_sources or so. Not sure we should add new methods. |
|
On Fri, Jul 3, 2015 at 10:18 AM, Denis A. Engemann <notifications@github.com
|
yes and plot_sources was the weakest of the plotting functions at least for raw. The evoked counterpart is really useful. |
|
... which reminds us of the fact that we should also recycle the epochs viz here. I would suspect that is at least as useful as the raw viz for ICA. |
|
+1 for recycling raw and epochs browser here.
|
|
Epoch plotter implemented. Is there any sense in marking bad epochs here? Should it be disabled? Also, is the exclude parameter needed? Ica object already has an exclude attribute, which is basically the same. Same goes for raw plotter. So basically the use case would change so that the user updates the ica.exclude before plotting to see the components marked for exclusion. |
|
No sense, so yes, it should be disabled
|
|
I changed the plot_sources to use these new functions for epochs and raw instances. See if you're happy. |
|
works great for me
did not review full code though.
Usability feedback. Not for this PR but it would be great to preview the
topography of source from the browser. I see some nice alpha waves on raw
examples and it's frustrating not to be able to see the associated
topography.
|
Did you mean like this? I implemented it here, since it was quite easy to do. |
Now it's a good moment to attack that (cc @teonlamont). I'd do it at the plotting level, not at the level of the sources as raw function. I'm still wondering though what's better. It's actually correct: ICA component #109 has the python index 108, wheras component #0 does not exist. We do the same for the channels btw. |
Yeah, I intuitively thought that the indexing should start from 1, but all the existing plotting functions seem to be using 0 for ICA. |
|
I guess this is ready for review, but it still needs some testing. |
|
closes #2000 #2043. also, what's the verdict on the indexing, are we plotting the ICs to start at 1 e.g. btw, the topo view is super useful |
…ually from example.
|
Ready for review. |
|
I am happy +1 for merge |
|
@jaeilepp we still don't see the EOG channels / ECG channels in the ICA time series, do we? I'll merge but please open an issue for later. Thanks for the wonderful work! |
|
great !
|
Here's a plotter for ICA components (item at #2213). The interactive selection doesn't work yet and neither does scrolling horizontally, but I wanted to show what I've done so far, because I'm not sure if this is what you guys wanted in the first place. I'm no expert in using ICA, so comments are welcome.
The plotter has basically the same interactive functions as does other browser functions. This PR also includes quite a bit of re-factoring of the existing browser functions.