Skip to content

MRG Butterfly epoch - #2206

Merged
dengemann merged 28 commits into
mne-tools:masterfrom
jaeilepp:butterfly-epoch
Jun 24, 2015
Merged

dengemann merged 28 commits into
mne-tools:masterfrom
jaeilepp:butterfly-epoch

Conversation

@jaeilepp

Copy link
Copy Markdown
Contributor

Butterfly plot can be toggled with a 'b' key.
closes #2205

@mainakjas

Copy link
Copy Markdown
Contributor

@jaeilepp can we have a help pop-up when you press ? a la gmail? I think @choldgraf @teonlamont and myself all agree that this would be useful.

@agramfort

Copy link
Copy Markdown
Member

can you share screenshots to it's easy to have a feeling of what you're working on?

@jaeilepp

Copy link
Copy Markdown
Contributor Author

Currently working on y axis...
screenshot from 2015-06-16 15 20 13

@agramfort

agramfort commented Jun 16, 2015 via email

Copy link
Copy Markdown
Member

@jaeilepp

Copy link
Copy Markdown
Contributor Author

There's a button for changing which channel type to view. Default view is the one with all channels.

@mainakjas

Copy link
Copy Markdown
Contributor

hmm ... do we really need a button? can't we just have it all there as three subplots?

@jaeilepp

Copy link
Copy Markdown
Contributor Author

can't we just have it all there as three subplots?

Good point. What about the other types (EOG etc.). Should they be shown too?

@agramfort

agramfort commented Jun 16, 2015 via email

Copy link
Copy Markdown
Member

@mainakjas

Copy link
Copy Markdown
Contributor

I was thinking more along the lines of evoked.plot() which has grad, mag and EEG nothing else. If we start looking at EOG, ECG, we have tons of other channel types ...

@jaeilepp

Copy link
Copy Markdown
Contributor Author

So what's the verdict? Three axes with mag, grad and eeg, fourth with eog or one for all existing types?

@mainakjas

Copy link
Copy Markdown
Contributor

I'll let @agramfort decide

@dengemann

Copy link
Copy Markdown
Member

Three axes with mag, grad and eeg, fourth with eog or one for all existing types?

Yes, this should be it!

@mainakjas

Copy link
Copy Markdown
Contributor

I don't quite like the toggle key to be 'b' because it doesn't make sense when you want to go back from the butterfly plot. How about something like spacebar (or any other better suggestions ...) ?

@choldgraf

Copy link
Copy Markdown
Contributor

+1 to a pop up infobox with possible key presses. Personally I think 'b' would be fine if the option were given as "turn butterfly plot on/off". Then it's just a toggle and semantically makes sense to be 'b' for me.

@jaeilepp jaeilepp changed the title WIP Butterfly epoch MRG Butterfly epoch Jun 17, 2015
@jaeilepp

Copy link
Copy Markdown
Contributor Author

Ready for review, I guess. Unless we want the infobox for this PR.

@dengemann

Copy link
Copy Markdown
Member

Unless we want the infobox for this PR.

infobox later.

@dengemann

Copy link
Copy Markdown
Member

@jaeilepp trying to run the example on your branch, it seems there are regressions,

/Users/dengemann/github/mne-python/examples/plot_from_raw_to_epochs_to_evoked.py in <module>()
     51 
     52 # Plot epochs.
---> 53 epochs.plot(trellis=False, block=True, fig_title='Auditory left/right')
     54 
     55 # Look at channels that caused dropped events, showing that the subject's

TypeError: plot() got an unexpected keyword argument 'fig_title'

Also the figure looks weird now:
https://www.dropbox.com/s/r9sl6c655bn3osc/Screenshot%202015-06-17%2012.34.38.png?dl=0

@dengemann

Copy link
Copy Markdown
Member

Butterfly view looks awesome!

I really much more liked the event ids on the top x axis, it was much cleaner.

@jaeilepp

Copy link
Copy Markdown
Contributor Author

I really much more liked the event ids on the top x axis, it was much cleaner.

Yeah, especially when there are a lot of epochs/view. I should probably change it back and deal with the layout problem of the title on another PR.

@teonbrooks

Copy link
Copy Markdown
Member

it'll be nice to have the infobox, I honestly have no clue what key does what other than the '-', '=', and '+'.

@mainakjas

Copy link
Copy Markdown
Contributor

yeah +100 on infobox

@agramfort

agramfort commented Jun 17, 2015 via email

Copy link
Copy Markdown
Member

@jaeilepp

Copy link
Copy Markdown
Contributor Author

screenshot from 2015-06-17 14 41 21

@mainakjas

Copy link
Copy Markdown
Contributor

The channel bar shouldn't be there any more no?

@mainakjas

Copy link
Copy Markdown
Contributor

how do I know which is grad, mag or EEG?

@mainakjas

Copy link
Copy Markdown
Contributor

no units on y axis?

@mainakjas

Copy link
Copy Markdown
Contributor

can we have it interactive so that you can click and know the channel name, just like in evoked.plot() ?

@teonbrooks

Copy link
Copy Markdown
Member

for the title, does that require a separate pr to fix? it's reverted to the clustering in the top. @dengemann my two concerns about the event id at the top is that it is clutters the title area at the moment, and it's not labeled. i know that @jaeilepp mentioned that to get the layout right with the title and the event ids at the top, he would need to rework the shared layout for both raw.plot and epochs.plot

@jaeilepp

Copy link
Copy Markdown
Contributor Author

can we have it interactive so that you can click and know the channel name, just like in evoked.plot() ?

What would be the button? Left click selects a bad epoch and right click draws a line.

@jaeilepp

Copy link
Copy Markdown
Contributor Author

We are running out of buttons. Left click is for bad epochs and right click for vertical lines.

@mainakjas

Copy link
Copy Markdown
Contributor

Can't we use left click on channel name?

@jaeilepp

Copy link
Copy Markdown
Contributor Author

Yeah, that's possible.

@mainakjas

Copy link
Copy Markdown
Contributor

ok let's do it then :)

@jaeilepp

Copy link
Copy Markdown
Contributor Author

Ready for review.

@teonbrooks

Copy link
Copy Markdown
Member

clicking to select bad channels is great! we will need that feature in raw.plot but that can be a separate pr.
the butterfly plot is slow for me :( what are people's opinions on just plot the max and min of the good channels with fill and leave the bads as it is.

@teonbrooks

Copy link
Copy Markdown
Member

just noticed this: dropping bads in the epochs viewer should update the info['bads']. this is the behavior we have in raw.plot.

@teonbrooks

Copy link
Copy Markdown
Member

sorry, my last comment was mistaken. the channel amplitudes changed from butterfly to channel view which made the view look different. I think we would want to keep the scaling of the two plots independent.

@agramfort

agramfort commented Jun 24, 2015 via email

Copy link
Copy Markdown
Member

@teonbrooks

Copy link
Copy Markdown
Member

@agramfort what are your thoughts on the butterfly plot just being the envelope of the signal. it might be time for me to upgrade my hardware but the browsing very sluggish b/c I think it's plotting all the lines in this view but that doesn't really give more information.

@agramfort

agramfort commented Jun 24, 2015 via email

Copy link
Copy Markdown
Member

@dengemann

Copy link
Copy Markdown
Member

@teonlamont I'd rather suggest to collect and implement a low performance config profile that can be selected optionally, e.g. for SSH or old computers.

@dengemann

Copy link
Copy Markdown
Member
with mne.viz.low_performance(**kwargs):
    epochs.plot()

@teonbrooks

Copy link
Copy Markdown
Member

ok cool. i will look into doing so. will be switching back to my desktop soon so this should become less of a problem.
only concern now is independent scaling for the plot. i'm actually ambivalent now so everyone likes it as is then i'm +1 then

@teonbrooks

Copy link
Copy Markdown
Member

oh, maybe a command line message to remind/notify the user of the drop channel similar to the drop epochs message. would be very useful.

@dengemann

Copy link
Copy Markdown
Member

only concern now is independent scaling for the plot.

What do you mean?

@jaeilepp

Copy link
Copy Markdown
Contributor Author

Just fixed it.

@teonbrooks

Copy link
Copy Markdown
Member

What do you mean?

scaling butterfly plot doesn't affect channel view and vice versa

@teonbrooks

Copy link
Copy Markdown
Member

for plotting the bad channels, can you make it so that it is in the background for all the channels. in the channel view, it is currently overlapping the previous channel.

@agramfort

Copy link
Copy Markdown
Member

Let's merge then

On 24 juin 2015, at 11:23, Teon L Brooks notifications@github.com wrote:

oh, maybe a command line message to remind/notify the user of the drop channel similar to the drop epochs message. would be very useful.

—
Reply to this email directly or view it on GitHub.

@jaeilepp

Copy link
Copy Markdown
Contributor Author

Yeah, ready from my part.

dengemann added a commit that referenced this pull request Jun 24, 2015
@dengemann
dengemann merged commit e855ca7 into mne-tools:master Jun 24, 2015
@dengemann

Copy link
Copy Markdown
Member

Thanks @jaeilepp !!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some minor epochs.plot issues

7 participants