Skip to content
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

ENH: dict features for Epochs objects #229

Closed
wants to merge 21 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
79 changes: 53 additions & 26 deletions mne/epochs.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,11 @@ class Epochs(object):
events : array, of shape [n_events, 3]
Returned by the read_events function

event_id : int | None
The id of the event to consider. If None all events are used.
event_id : int | dict
The id of the event to consider. If dict,
the keys can later be used to acces associated events. Example:
dict(auditory=1, visual=3). If int, a dict will be created with
the str value resulting from the name parameter as key.

tmin : float
Start time before event
Expand Down Expand Up @@ -105,6 +108,9 @@ class Epochs(object):
info: dict
Measurement info

event_ids : dict
Labels mapped to event_ids.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Names of conditions corresponding to event_ids.

Labels as a particular meaning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


ch_names: list of string
List of channels' names

Expand Down Expand Up @@ -159,10 +165,15 @@ def __init__(self, raw, events, event_id, tmin, tmax, baseline=(None, 0),

self.raw = raw
self.verbose = raw.verbose if verbose is None else verbose
self.event_id = event_id
self.name = name
if isinstance(event_id, dict):
if not all([isinstance(v, int) for v in event_id.values()]):
raise ValueError('Events must be of type integer')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checks also that keys are strings otherwise it will conflict with the getattr that are integers or a slice used to select some epochs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, thanks!

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

In mne/epochs.py:

@@ -159,10 +165,15 @@ def init(self, raw, events, event_id, tmin, tmax, baseline=(None, 0),

     self.raw = raw
     self.verbose = raw.verbose if verbose is None else verbose
  •    self.event_id = event_id
    
  •    self.name = name
    
  •    if isinstance(event_id, dict):
    
  •        if not all([isinstance(v, int) for v in event_id.values()]):
    
  •            raise ValueError('Events must be of type integer')
    
    checks also that keys are strings otherwise it will conflict with the getattr that are integers or a slice used to select some epochs


Reply to this email directly or view it on GitHub.

self.event_ids = event_id
else:
self.event_ids = {name: event_id}
self.tmin = tmin
self.tmax = tmax
self.name = name
self.keep_comp = keep_comp
self.dest_comp = dest_comp
self.baseline = baseline
Expand Down Expand Up @@ -209,10 +220,11 @@ def __init__(self, raw, events, event_id, tmin, tmax, baseline=(None, 0),

# Select the desired events
self.events = events
self._events = {}
if event_id is not None:
selected = np.logical_and(events[:, 1] == 0,
events[:, 2] == event_id)
self.events = self.events[selected]
overlap = np.in1d(events[:, 2], event_id.values())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use in1d from fixes.py as np.in1d is too recent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, i remember we had this issue.

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

In mne/epochs.py:

     if event_id is not None:
  •        selected = np.logical_and(events[:, 1] == 0,
    
  •                                  events[:, 2] == event_id)
    
  •        self.events = self.events[selected]
    
  •        overlap = np.in1d(events[:, 2], event_id.values())
    
    use in1d from fixes.py as np.in1d is too recent


Reply to this email directly or view it on GitHub.

selected = np.logical_and(events[:, 1] == 0, overlap)
self.events = events[selected]

n_events = len(self.events)

Expand Down Expand Up @@ -463,26 +475,36 @@ 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if not isinstance(key, str):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A suggestion to simplify this decision tree:

if isinstance(key, str): convert key to slice/index

procede like for slice/index keys

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christianmbrdobeck @agramfort @mluessi -- if the Epochs / copy crew
doesen't see any further serious issues I would suggest to refocus on the
bigger picture and finalize this PR.

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

wrote:

In mne/epochs.py:

     if self.preload:
  •        if isinstance(key, slice):
    
  •            epochs._data = self._data[key]
    
  •        else:
    
  •            key = np.atleast_1d(key)
    
  •            epochs._data = self._data[key]
    
  •        self._data = data
    
  •    if not isinstance(key, str):
    

A suggestion to simplify this decision tree:

if isinstance(key, str): convert key to slice/index

procede like for slice/index keys


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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I had in mind:

    if isinstance(key, str):
        name = (key if epochs.name == 'Unknown' else 'epochs_%s' % key)
        key = (self.events[0:, 2] == self.event_id[key])
    else:
        name = self.name

    epochs.events = np.atleast_2d(self.events[key])
    epochs.name = name
    if self.preload:
        select = key if isinstance(key, slice) else np.atleast_1d(key)
        epochs._data = epochs._data[select]

    return epochs

you can get rid of ._get_events()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me see what I can do.

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

wrote:

In mne/epochs.py:

     if self.preload:
  •        if isinstance(key, slice):
    
  •            epochs._data = self._data[key]
    
  •        else:
    
  •            key = np.atleast_1d(key)
    
  •            epochs._data = self._data[key]
    
  •        self._data = data
    
  •    if not isinstance(key, str):
    

What I had in mind:

if isinstance(key, str):
    name = (key if epochs.name == 'Unknown' else 'epochs_%s' % key)
    key = (self.events[0:, 2] == self.event_id[key])
else:
    name = self.name

epochs.events = np.atleast_2d(self.events[key])
epochs.name = name
if self.preload:
    select = key if isinstance(key, slice) else np.atleast_1d(key)
    epochs._data = epochs._data[select]

return epochs

you can get rid of ._get_events()


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

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.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_ids:
raise KeyError('Event "%s" is not in Epochs.' % key)
else:
epochs.events = self._get_events(key)
if self.preload:
epochs._data[epochs.events[:, 0]]

if self.preload:
if isinstance(key, slice):
epochs._data = self._data[key]
else:
key = np.atleast_1d(key)
epochs._data = self._data[key]
return epochs

def average(self, picks=None):
"""Compute average of epochs

Parameters
----------

picks: None | array of int
If None only MEG and EEG channels are kept
otherwise the channels indices in picks are kept.
Expand All @@ -492,6 +514,7 @@ def average(self, picks=None):
evoked : Evoked instance
The averaged epochs
"""

return self._compute_mean_or_stderr(picks, 'ave')

def standard_error(self, picks=None):
Expand All @@ -510,12 +533,18 @@ def standard_error(self, picks=None):
"""
return self._compute_mean_or_stderr(picks, 'stderr')

def _get_events(self, event_name):
"""Aux function: get event ids"""
ids = None
if event_name in self.event_ids:
ids = self.events[self.events[0:, 2] == self.event_ids[event_name]]

return ids

def _compute_mean_or_stderr(self, picks, mode='ave'):
"""Compute the mean or std over epochs and return Evoked"""
if mode == 'stderr':
_do_std = True
else:
_do_std = False

_do_std = True if mode == 'stderr' else False
evoked = Evoked(None)
evoked.info = cp.deepcopy(self.info)
n_channels = len(self.ch_names)
Expand Down Expand Up @@ -643,9 +672,8 @@ def resample(self, sfreq, npad=100, window='boxcar'):
def copy(self):
""" Return copy of Epochs instance
"""
raw = self.raw.copy()
new = deepcopy(self)
new.raw = raw
new.raw = self.raw.copy()
return new

def save(self, fname):
Expand Down Expand Up @@ -704,7 +732,6 @@ def save(self, fname):
end_block(fid, FIFF.FIFFB_MEAS)
end_file(fid)


def as_data_frame(self, frame=True):
"""Get the epochs as Pandas panel of data frames

Expand Down