ENH: dict features for Epochs objects #229

dengemann · 2012-12-05T19:21:56Z

parameter event_id now takes either int or dict. Internalla a dict of event_label : event_id mappings will be created which are exposed to the user.
On init the events passed are reduced to the events available in the event_id dict. The dict is then used to subset the available events
for this purpose I added a private function ._get_events() that is about compliant with Python dictionary .get --- returns None, if not in event_ids, else it returns the events associated with the key.
I found instead of passing keys to average / std_error, etc. It is less testing affordant and cleaner to extend .get_item. It now checks, whether the key is an int or str. if int, as before subsettig by epoch number, if str and in self.event_ids an Epochs subset associated with the key is returned.

To get what @agramfort initially wanted he now can say epochs['audio'].average()

A simple test case is:

run examples/plot_read_and_write_raw_data.py

import mne

events = mne.find_events(raw)

epochs = mne.Epochs(raw, events, dict(a=1, v=2, button=32), tmin=-0.2, tmax=0.5)

epochs['button']

epochs['b']

epochs['a'].average().plot()

Let me know what you think about this take befor I get into further details

D

Related to mne-tools#133 -- please don't merge. I wanted to expose one possible approach to this and would like to get some feedback before proceeding.

@agramfort

- parameter event_id now takes either int or dict. Internalla a dict of event_label : event_id mappings will be created which are exposed to the user. - On init the events passed are reduced to the events available in the event_id dict. The dict is then used to subset the available events - for this purpose I added a private function ._get_events() that is about compliant with Python dictionary .get --- returns None, if not in event_ids, else it returns the events associated with the key. - I found instead of passing keys to average / std_error, etc. It is less testing affordant and cleaner to extend .__get_item__. It now checks, whether the key is an int or str. if int, as before subsettig by epoch number, if str and in self.event_ids an Epochs subset associated with the key is returned. To get what @agramfort initially wanted he now can say epochs['audio'].average() A simple test case is: ```Python run examples/plot_read_and_write_raw_data.py import mne events = mne.find_events(raw) epochs = mne.Epochs(raw, events, dict(a=1, v=2, button=32), tmin=-0.2, tmax=0.5) epochs['button'] epochs['b'] epochs['a'].average().plot() ``` Let me know what you think about this take befor I get into further details D

agramfort · 2012-12-05T20:47:25Z

ok done ...

agramfort · 2012-12-05T20:53:52Z

mne/epochs.py

@@ -105,6 +108,9 @@ class Epochs(object):
    info: dict
        Measurement info

+    event_ids : dict
+        Labels mapped to event_ids.


Names of conditions corresponding to event_ids.

Labels as a particular meaning.

right, would mix up things with stc..

On 05.12.2012, at 21:53, Alexandre Gramfort notifications@github.com wrote:

In mne/epochs.py:

@@ -105,6 +108,9 @@ class Epochs(object):
info: dict
Measurement info

event_ids : dict

Labels mapped to event_ids.
Names of conditions corresponding to event_ids.

Labels as a particular meaning.

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

agramfort · 2012-12-05T20:56:44Z

can you add tests and example ?

dengemann · 2012-12-05T20:57:45Z

sure, let me run home first ;-)

On 05.12.2012, at 21:56, Alexandre Gramfort notifications@github.com wrote:

can you add tests and example ?

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

dengemann · 2012-12-05T21:05:15Z

Do you think this deserves an individual example or should we just rewrite the examples in which conditions are being compared?

what about deprecation?
as we diden't change param-keywords and the old functionality is / should be preserved we should be fine, right?

On 05.12.2012, at 21:56, Alexandre Gramfort notifications@github.com wrote:

can you add tests and example ?

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

agramfort · 2012-12-05T21:10:04Z

Do you think this deserves an individual example or should we just rewrite the examples in which conditions are being compared?

What I have in mind is topo plot of the ERP/ERF in two conditions

mne.viz.plot_topo_evoked([epochs['visual'].average(),
epochs['audio'].average()], tmin…, scalings…)

that would show a difference in the ERPs.

We could even do a 2 sample T-test topo using the standard deviation
computation :)

what about deprecation?
as we diden't change param-keywords and the old functionality is / should be preserved we should be fine, right?

you should not need to deprecate anything. You did not break anything
and I think we should support the current API.

dengemann · 2012-12-05T21:18:19Z

On 05.12.2012, at 22:10, Alexandre Gramfort notifications@github.com wrote:

Do you think this deserves an individual example or should we just rewrite the examples in which conditions are being compared?

What I have in mind is topo plot of the ERP/ERF in two conditions

mne.viz.plot_topo_evoked([epochs['visual'].average(),
epochs['audio'].average()], tmin…, scalings…)

that would show a difference in the ERPs.

We could even do a 2 sample T-test topo using the standard deviation
computation :)

exactly!

as we catch up with misc support all this could take place in ICA space --> select responding sources.

also, related to #133 the architecture proposed is easily extendable without breaking things. one could pass tuples to get combinations of conditions. do you by chance know whether formula objects are hashable?

one more point. the pandas exporter -- after some edits-- could now export proper dataframes you can meaningfully pass to patsy / statsmodels / R.

what about deprecation?
as we diden't change param-keywords and the old functionality is / should be preserved we should be fine, right?

you should not need to deprecate anything. You did not break anything
and I think we should support the current API.
we do so.
—
Reply to this email directly or view it on GitHub.

dengemann · 2012-12-05T21:37:15Z

On 05.12.2012, at 22:10, Alexandre Gramfort notifications@github.com wrote:

Do you think this deserves an individual example or should we just rewrite the examples in which conditions are being compared?

What I have in mind is topo plot of the ERP/ERF in two conditions

mne.viz.plot_topo_evoked([epochs['visual'].average(),
epochs['audio'].average()], tmin…, scalings…)

that would show a difference in the ERPs.

We could even do a 2 sample T-test topo using the standard deviation
computation :)

and once more this calls for support of repeated measures anova...

what about deprecation?
as we diden't change param-keywords and the old functionality is / should be preserved we should be fine, right?

you should not need to deprecate anything. You did not break anything
and I think we should support the current API.
—
Reply to this email directly or view it on GitHub.

@agramfort

…mple @agramfort : if you're fine with this let's merge it soon and get back to the fancy stats / 2-sample t-tests / on the next iteration. I think for 0.5 this is already nice whereas other parts of mne need to be polished for 0.5.

dengemann · 2012-12-05T23:32:25Z

So for the moment I think we're done with this. Let's merge this soon and go deeper as the other pending construction sites are supplied with polish and care.

christianbrodbeck · 2012-12-06T04:40:16Z

mne/epochs.py

-
-        epochs = cp.copy(self)  # XXX : should use deepcopy but breaks ...
-        epochs.events = np.atleast_2d(self.events[key])
+        epochs = self.copy()


If I'm not mistaken this produces a deepcopy of all the data even before indexing, which would not be very efficient. Also, should users modify the Epochs' ._data? If not, why not let numpy decide when to create deep copies of the data (and internally only deepcopy before actually modifying the data)?

you're right. But otherwise it means doing the copy manually which is more work to do right.

it might work with a trick like:

data = self._data del self._data epochs = self.copy() self._data = data etc.

And hope that self.copy() never crashes :)

Eventually it might be cleaner to have an Epochs class that can be initialized with its attributes... and that might also be useful for the result of data transformations such as time-frequency?

Maybe but it's a huge API break and I think we can live with that now don't you think?

Yes, hence "eventually" :) If you have a NewEpochs class that initializes with data, then the current Epochs could be transformed into a function that create such objects to keep the api backwards compatible (or subclass to make type-checking work as well).

But wait, all is fine. If not preloaded no copy of self._data because it
doesn't exist.
else it in fact gets sliced to the events selected by the key. Where is the
issue?

self.copy() makes a deep copy of self, including self._data. For consistency with numpy this seems a good idea. However, when indexing this entails a lot of unnecessary copying (especially since you're going to discard most of the copied data anyways when you index).

Maybe temporarily remove self._data in getitem before calling self.copy()?

I see your point, but I'm still a bit reluctant to add something like an
index argument to copy. Or do you see a way to do it without passing an
index argument?

On Thu, Dec 6, 2012 at 5:37 PM, Christian Brodbeck <notifications@github.com

wrote:

In mne/epochs.py:

@@ -463,19 +493,30 @@ def repr(self):
def getitem(self, key):
"""Return an Epochs object with a subset of epochs
"""

if not self._bad_dropped:

warnings.warn("Bad epochs have not been dropped, indexing will be "

- "inaccurate. Use drop_bad_epochs() or preload=True")

epochs = cp.copy(self) # XXX : should use deepcopy but breaks ...

epochs.events = np.atleast_2d(self.events[key])

epochs = self.copy()

But wait, all is fine. If not preloaded no copy of self._data because it

doesn't exist.
else it in fact gets sliced to the events selected by the key. Where is the
issue?

self.copy() makes a deep copy of self, including self._data. For
consistency with numpy this seems a good idea. However, when indexing this
entails a lot of unnecessary copying (especially since you're going to
discard most of the copied data anyways when you index).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/229/files#r2337289.

Yes sounds reasonable. More than having a private ._copy with index arg...
Or what do you / the others think?

On Thu, Dec 6, 2012 at 5:40 PM, Christian Brodbeck <notifications@github.com

wrote:

In mne/epochs.py:

@@ -463,19 +493,30 @@ def repr(self):
def getitem(self, key):
"""Return an Epochs object with a subset of epochs
"""

if not self._bad_dropped:

warnings.warn("Bad epochs have not been dropped, indexing will be "

- "inaccurate. Use drop_bad_epochs() or preload=True")

epochs = cp.copy(self) # XXX : should use deepcopy but breaks ...

epochs.events = np.atleast_2d(self.events[key])

epochs = self.copy()

Maybe temporarily remove self._data in getitem before calling
self.copy()?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/229/files#r2337336.

Added the version you proposed plus fixed a missing assignment-statement.
The epochs tests seem to run smoothly.

On Thu, Dec 6, 2012 at 5:41 PM, Denis-Alexander Engemann <
denis.engemann@gmail.com> wrote:

Yes sounds reasonable. More than having a private ._copy with index arg...
Or what do you / the others think?

On Thu, Dec 6, 2012 at 5:40 PM, Christian Brodbeck <
notifications@github.com> wrote:

In mne/epochs.py:

@@ -463,19 +493,30 @@ def repr(self):
def getitem(self, key):
"""Return an Epochs object with a subset of epochs
"""

if not self._bad_dropped:

warnings.warn("Bad epochs have not been dropped, indexing will be "

- "inaccurate. Use drop_bad_epochs() or preload=True")

epochs = cp.copy(self) # XXX : should use deepcopy but breaks ...

epochs.events = np.atleast_2d(self.events[key])

epochs = self.copy()

Maybe temporarily remove self._data in getitem before calling
self.copy()?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/229/files#r2337336.

dengemann · 2012-12-06T09:12:35Z

wait, one more iteration... found some more things to fix.

agramfort · 2012-12-06T09:34:52Z

mne/epochs.py

@@ -30,6 +30,7 @@
 from .filter import resample
 from .event import _read_events_fif
 from . import verbose
+from .fixes import _in1d


no in1d so if np.in1d is present use it.

Yes, check for it -- was a bit late yesterday ;-)

dengemann · 2012-12-06T12:19:38Z

Wait one more moment, I now have a failing test in test_epochs --- test_bootstrap() -- let me see what's going on there...

- made sure the API does not get changed, went back to epochs.event_id as attribute instead of .event_ids. - restored support for epochs with event_id=None, in this case the internal event id dict is just the event_id with str(event_id) as key - for the sake of consistency and to make the read-write test work again, the default key for event_id=int is str(event_id) -- as above. - replaced cp.deepcopy with self.copy whereever it was possible. Bootstrapping did not work anymore..

dengemann · 2012-12-06T13:41:45Z

Ok, looks good now. Feel free to review / merge!

agramfort · 2012-12-06T14:16:38Z

it would be nice to illustrate this new features in the python_tutorial.rst or/and in at least one example.

Do you feel brave enough to do the mne.viz.plot_topo_evoked to illustrate it?

dengemann · 2012-12-06T14:21:47Z

Already pushed it yesterday evening, isin't it up?

On Thu, Dec 6, 2012 at 3:16 PM, Alexandre Gramfort <notifications@github.com

wrote:

it would be nice to illustrate this new features in the
python_tutorial.rst or/and in at least one example.

Do you feel brave enough to do the mne.viz.plot_topo_evoked to illustrate
it?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/229#issuecomment-11086943.

dengemann · 2012-12-06T14:24:47Z

Ok, now it's up

On Thu, Dec 6, 2012 at 3:21 PM, Denis-Alexander Engemann <
denis.engemann@gmail.com> wrote:

Already pushed it yesterday evening, isin't it up?

On Thu, Dec 6, 2012 at 3:16 PM, Alexandre Gramfort <
notifications@github.com> wrote:

it would be nice to illustrate this new features in the
python_tutorial.rst or/and in at least one example.

Do you feel brave enough to do the mne.viz.plot_topo_evoked to illustrate
it?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/229#issuecomment-11086943.

christianbrodbeck · 2012-12-06T17:21:26Z

mne/epochs.py

+            if self.preload:
+                epochs._data = epochs._data[epochs.events[:, 0]]
+            epochs.name = (key if epochs.name == 'Unknown'
+                           else 'epochs_%s' % key)


copying looks good to me now.

Great, thanks heaps for the detailed feedback.

On Thu, Dec 6, 2012 at 6:21 PM, Christian Brodbeck <notifications@github.com

wrote:

In mne/epochs.py:

epochs.events = np.atleast_2d(self.events[key]
)
+

if self.preload:

if isinstance(key, slice):

epochs._data = self._data[key]

else:

key = np.atleast_1d(key)

epochs._data = self._data[key]

elif key not in self.event_id:

raise KeyError('Event "%s" is not in Epochs.' % key)

else:

epochs.events = self._get_events(key)

if self.preload:

epochs._data = epochs._data[epochs.events[:, 0]]

epochs.name = (key if epochs.name == 'Unknown'

else 'epochs_%s' % key)

copying looks good to me now.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/229/files#r2337924.

And COSMITS in example

agramfort · 2012-12-06T18:23:05Z

examples/plot_topo_compare_conditions.py

+                                            include=include, exclude=exclude)
+
+# Create epochs including different events
+epochs = mne.Epochs(raw, events, dict(audio_l=1, visual_r=3), tmin, tmax, picks=picks,


pep8 :) line too long

agramfort · 2012-12-06T18:27:29Z

LGTM besides the detail on the example plot.

dengemann · 2012-12-06T18:28:42Z

I'll see how to make use of the new cool color function.

On Thu, Dec 6, 2012 at 7:27 PM, Alexandre Gramfort <notifications@github.com

wrote:

LGTM besides the detail on the example plot.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/229#issuecomment-11097448.

- fixing cosmits - drafting list support for plot_topo (DON'T merge yet)

@agramfort

+ updating the compare conditions example @agramfort, hopefully I left some tiny nits to pick for you. Feel free to edit if something is fishy; otherwise please merge!

dengemann · 2012-12-06T22:34:52Z

Btw. @mluessi (and @agramfort ; can you confirm this?) --- as much as I love the most recent call back function --- one thing which is annoying is that for some reason apple track pad gestures by some low level mechanism get translated to clicks, so if you happen to swipe in proximity to the topo plot it may happen that n_channels tiny figures would pop up. Is this somehow avoidable? I think we should catch this before the upcoming release.

dengemann · 2012-12-06T22:42:13Z

@mluessi @agramfort JTLYK what I'm talking about: 222 with one swipe ;-)

dengemann · 2012-12-06T22:42:21Z

https://www.dropbox.com/s/855xz1dpgp17jgg/Screen%20Shot%202012-12-06%20at%2011.39.45%20PM.png

mluessi · 2012-12-07T02:20:23Z

I can't confirm it fortunately I don't have to use a Mac :-). If it is really a problem we could disable it for OS-X or add an option..

agramfort · 2012-12-07T07:54:38Z

examples/plot_topo_compare_conditions.py

+pl.close('all')
+pl.figure()
+title = 'MNE sample data - left auditory and visual'
+plot_topo(evokeds, layout, color=['y', 'g'], title=title)


apparently there is a bug if you don't give color then only one evoked is plotted.

I'm addings some cases + guessing.
If colors is str, always use that colour,
if None, cycle through list of colors.

Is there btw. a prefered order? 'w' 'y' 'g' 'r' 'b' ? if not I would just
take the COLORS list specified in viz and prepend 'w'

On Fri, Dec 7, 2012 at 8:54 AM, Alexandre Gramfort <notifications@github.com

wrote:

In examples/plot_topo_compare_conditions.py:

+# Create epochs including different events
+epochs = mne.Epochs(raw, events, dict(audio_l=1, visual_r=3), tmin, tmax,

picks=picks, baseline=(None, 0), reject=reject)

+# Generate list of evoked objects from conditions names
+evokeds = [epochs[name].average() for name in 'audio_l', 'visual_r']
+
+###############################################################################
+# Show topography for two different conditions
+
+layout = read_layout('Vectorview-all.lout')
+
+pl.close('all')
+pl.figure()
+title = 'MNE sample data - left auditory and visual'
+plot_topo(evokeds, layout, color=['y', 'g'], title=title)

apparently there is a bug if you don't give color then only one evoked is
plotted.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/229/files#r2345728.

I'm addings some cases + guessing. If colors is str, always use that colour, if None, cycle through list of colors. Is there btw. a prefered order? 'w' 'y' 'g' 'r' 'b' ? if not I would just take the COLORS list specified in viz and prepend 'w'

itertools.cycle on ['w' 'y' 'g' 'r' 'b'] is good to me.

indeed. I can reproduce the swiping… darn...

see recent commit

On Fri, Dec 7, 2012 at 10:26 AM, Alexandre Gramfort <
notifications@github.com> wrote:

In examples/plot_topo_compare_conditions.py:

+# Create epochs including different events
+epochs = mne.Epochs(raw, events, dict(audio_l=1, visual_r=3), tmin, tmax,

picks=picks, baseline=(None, 0), reject=reject)

+# Generate list of evoked objects from conditions names
+evokeds = [epochs[name].average() for name in 'audio_l', 'visual_r']
+
+###############################################################################
+# Show topography for two different conditions
+
+layout = read_layout('Vectorview-all.lout')
+
+pl.close('all')
+pl.figure()
+title = 'MNE sample data - left auditory and visual'
+plot_topo(evokeds, layout, color=['y', 'g'], title=title)

I'm addings some cases + guessing. If colors is str, always use that
colour, if None, cycle through list of colors. Is there btw. a prefered
order? 'w' 'y' 'g' 'r' 'b' ? if not I would just take the COLORS list
specified in viz and prepend 'w'
itertools.cycle on ['w' 'y' 'g' 'r' 'b'] is good to me. indeed. I can
reproduce the swiping… darn...

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/229/files#r2346337.

agramfort · 2012-12-07T07:58:01Z

besides my last commnets LGTM

and everything works fine on my mac with EPD.

In [3]: import matplotlib

In [4]: matplotlib.__version__
Out[4]: '1.1.0'

dengemann · 2012-12-07T08:46:08Z

.... but can you reproduce the swiping and opening hundreds of of plots
effect (see post from above)?

On Fri, Dec 7, 2012 at 8:58 AM, Alexandre Gramfort <notifications@github.com

wrote:

besides my last commnets LGTM

and everything works fine on my mac with EPD.

In [3]: import matplotlib

In [4]: matplotlib.version
Out[4]: '1.1.0'

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/229#issuecomment-11121855.

- add stuff to what's new - more sophisticated color handling fot plot_topo

agramfort · 2012-12-07T09:59:45Z

merged by rebase but we have a tiny problem when clicking and the line is white as we don't see it with a white background....

agramfort · 2012-12-07T09:59:59Z

thanks !

I almost forgot :)

dengemann added 2 commits December 5, 2012 18:59

ENH (DRAFT): add dict features and support for conditions to Epochs

95a7f75

Related to mne-tools#133 -- please don't merge. I wanted to expose one possible approach to this and would like to get some feedback before proceeding.

dengemann mentioned this pull request Dec 5, 2012

epochs event_id as dict and support for multiple marker numbers #133

Closed

agramfort reviewed Dec 5, 2012
View reviewed changes

dengemann added 2 commits December 5, 2012 23:13

ENH: improving docs, adding catches + tests

14d32ed

christianbrodbeck reviewed Dec 6, 2012
View reviewed changes

agramfort reviewed Dec 6, 2012
View reviewed changes

dengemann added 3 commits December 6, 2012 11:54

COSMITS + FIXES

2755b1c

ENH: yet another exception handler for event_id

d0db669

FIX epochs test

7818662

ENH: example... forgot to add it.

9b8e023

FIX: fixing last ENH.

f17b4a4

christianbrodbeck reviewed Dec 6, 2012
View reviewed changes

ENH: Neater and cleaner decision tree for @christianmbrodbeck

f6cefe6

And COSMITS in example

agramfort reviewed Dec 6, 2012
View reviewed changes

dengemann added 2 commits December 6, 2012 20:18

ENH: dropping unnecessary _get_events for @agramfort

3381f0c

- fixing cosmits - drafting list support for plot_topo (DON'T merge yet)

ENH: adding support for list of eveoks to plot_topo.

451d701

+ updating the compare conditions example @agramfort, hopefully I left some tiny nits to pick for you. Feel free to edit if something is fishy; otherwise please merge!

dengemann added 3 commits December 7, 2012 01:02

COSMITS

a6fde9b

COSMITS_II

05790d6

COSMITS_III + remove fixing stuff

b60033e

agramfort reviewed Dec 7, 2012
View reviewed changes

CLEANUP:

609728d

- add stuff to what's new - more sophisticated color handling fot plot_topo

agramfort closed this Dec 7, 2012

dengemann mentioned this pull request Dec 7, 2012

ENH: Generalize epoch images to handle custom data (misc) #221

Merged

This was referenced Dec 7, 2012

FIX: click & shift to open figure #234

Closed

Epochs dict features don't work with save to fiff #238

Closed

ENH: dict features for Epochs objects #229

ENH: dict features for Epochs objects #229

Conversation

dengemann commented Dec 5, 2012

agramfort commented Dec 5, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agramfort commented Dec 5, 2012

dengemann commented Dec 5, 2012

dengemann commented Dec 5, 2012

agramfort commented Dec 5, 2012

dengemann commented Dec 5, 2012

dengemann commented Dec 5, 2012

dengemann commented Dec 5, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

- "inaccurate. Use drop_bad_epochs() or preload=True")

Choose a reason for hiding this comment

- "inaccurate. Use drop_bad_epochs() or preload=True")

Choose a reason for hiding this comment

- "inaccurate. Use drop_bad_epochs() or preload=True")

dengemann commented Dec 6, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dengemann commented Dec 6, 2012

dengemann commented Dec 6, 2012

agramfort commented Dec 6, 2012

dengemann commented Dec 6, 2012

dengemann commented Dec 6, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agramfort commented Dec 6, 2012

dengemann commented Dec 6, 2012

dengemann commented Dec 6, 2012

dengemann commented Dec 6, 2012

dengemann commented Dec 6, 2012

mluessi commented Dec 7, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agramfort commented Dec 7, 2012

dengemann commented Dec 7, 2012

agramfort commented Dec 7, 2012

agramfort commented Dec 7, 2012