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
Show file tree
Hide file tree
Changes from 14 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
65 changes: 65 additions & 0 deletions examples/plot_topo_compare_conditions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
"""
===============================================
Compare Evoked Reponses to Different Conditions
===============================================

"""

# Authors: Denis Engemann <d.engemann@fz-juelich.de>
# Alexandre Gramfort <gramfort@nmr.mgh.harvard.edu>
#
# License: BSD (3-clause)

print __doc__

import pylab as pl
import mne

from mne.fiff import Raw, pick_types
from mne.layouts import read_layout
from mne.viz import plot_topo
from mne.datasets import sample
data_path = sample.data_path('.')

###############################################################################
# Set parameters
raw_fname = data_path + '/MEG/sample/sample_audvis_filt-0-40_raw.fif'
event_fname = data_path + '/MEG/sample/sample_audvis_filt-0-40_raw-eve.fif'
event_id = 1
tmin = -0.2
tmax = 0.5

# Setup for reading the raw data
raw = Raw(raw_fname)
events = mne.read_events(event_fname)

# Set up pick list: EEG + STI 014 - bad channels (modify to your needs)
include = [] # or stim channels ['STI 014']
exclude = raw.info['bads'] + ['EEG 053'] # bads + 1 more

# pick EEG channels
picks = pick_types(raw.info, meg=True, eeg=False, stim=False, eog=True,
include=include, exclude=exclude)
# Read epochs
epochs = mne.Epochs(raw, events, dict(audio_l=1, visual_r=3), tmin, tmax, picks=picks,
Copy link
Member

Choose a reason for hiding this comment

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

pep8 :) line too long

baseline=(None, 0), reject=dict(eog=150e-6))
Copy link
Member

Choose a reason for hiding this comment

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

why do rejection for meg channesl?

Copy link
Member Author

Choose a reason for hiding this comment

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

Beacause it was late and I copy and pasted ;-)

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

wrote:

In examples/plot_topo_compare_conditions.py:

+# Setup for reading the raw data
+raw = Raw(raw_fname)
+events = mne.read_events(event_fname)
+
+# Set up pick list: EEG + STI 014 - bad channels (modify to your needs)
+include = [] # or stim channels ['STI 014']
+exclude = raw.info['bads'] + ['EEG 053'] # bads + 1 more
+
+# pick EEG channels
+picks = pick_types(raw.info
, meg=True, eeg=False, stim=False, eog=True,

  •                                        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,
  •              baseline=(None, 0), reject=dict(eog=150e-6))
    

why do rejection for meg channesl?


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


# access sub-epochs by conditions labels
epochs_au, epochs_vi = epochs['audio_l'], epochs['visual_r']

print epochs_au, epochs_au

layout = read_layout('Vectorview-all')

evoked_au, evoked_vi = epochs_au.average(), epochs_vi.average()


###############################################################################
# Show topography for two different conditions

pl.close('all')
for evoked in [evoked_au, evoked_vi]:
pl.figure()
title = 'MNE sample data (condition : %s)' % evoked.comment
plot_topo(evoked, layout, title=title)
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 is to be able to pass a list of evoked to plot all of them. In white, yellow, green, ... the mne_analyze way.

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 know, just that plot_topo doesn't support it..

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

wrote:

In examples/plot_topo_compare_conditions.py:

+print epochs_au, epochs_au
+
+layout = read_layout('Vectorview-all')
+
+evoked_au, evoked_vi = epochs_au.average(), epochs_vi.average()
+
+
+###############################################################################
+# Show topography for two different conditions
+
+pl.close('all')
+for evoked in [evoked_au, evoked_vi]:

  • pl.figure()
  • title = 'MNE sample data (condition : %s)' % evoked.comment
  • plot_topo(evoked, layout, title=title)

what I had in mind is to be able to pass a list of evoked to plot all of
them. In white, yellow, green, ... the mne_analyze way.


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

Copy link
Member

Choose a reason for hiding this comment

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

I know that's why asked :)

I'll do it tomorrow if nobody does it before.

thanks heaps for this new cool feature !

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

As to the plot. So what you have in mind is to iterate over channels and
plot e.g. two conditions inside a channel window rendered in MNE analyze
colors?

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

wrote:

In examples/plot_topo_compare_conditions.py:

+print epochs_au, epochs_au
+
+layout = read_layout('Vectorview-all')
+
+evoked_au, evoked_vi = epochs_au.average(), epochs_vi.average()
+
+
+###############################################################################
+# Show topography for two different conditions
+
+pl.close('all')
+for evoked in [evoked_au, evoked_vi]:

  • pl.figure()
  • title = 'MNE sample data (condition : %s)' % evoked.comment
  • plot_topo(evoked, layout, title=title)

I know that's why asked :)

I'll do it tomorrow if nobody does it before.

thanks heaps for this new cool feature !


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

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Le 6 déc. 2012 à 19:38, "Denis A. Engemann" notifications@github.com a écrit :

In examples/plot_topo_compare_conditions.py:

+print epochs_au, epochs_au
+
+layout = read_layout('Vectorview-all')
+
+evoked_au, evoked_vi = epochs_au.average(), epochs_vi.average()
+
+
+###############################################################################
+# Show topography for two different conditions
+
+pl.close('all')
+for evoked in [evoked_au, evoked_vi]:

  • pl.figure()
  • title = 'MNE sample data (condition : %s)' % evoked.comment
  • plot_topo(evoked, layout, title=title)
    Sure! As to the plot. So what you have in mind is to iterate over channels and plot e.g. two conditions inside a channel window rendered in MNE analyze colors?


    Reply to this email directly or view it on GitHub.

pl.show()
120 changes: 92 additions & 28 deletions mne/epochs.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
from .filter import resample
from .event import _read_events_fif
from . import verbose
from .fixes import in1d


class Epochs(object):
Expand All @@ -43,8 +44,13 @@ 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 | None
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 id as string. If None, all events will be used with
and a dict is created with string integer names corresponding
to the event id integers.

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

event_id : dict
Names of of conditions corresponding to event_ids.

ch_names: list of string
List of channels' names

Expand All @@ -121,7 +130,7 @@ class Epochs(object):
get_data() : self
Return all epochs as a 3D array [n_epochs x n_channels x n_times].

average(picks=None) : self
average(picks=None) : Evoked
Return Evoked object containing averaged epochs as a
2D array [n_channels x n_times].

Expand All @@ -139,6 +148,14 @@ class Epochs(object):
resample() : self, int, int, int, string or list
Resample preloaded data.

as_data_frame() : DataFrame
Export Epochs object as Pandas DataFrame for subsequent statistical
analyses.

to_nitime() : TimeSeries
Export Epochs object as nitime TimeSeries object for subsequent
analyses.

Notes
-----
For indexing and slicing:
Expand All @@ -148,6 +165,13 @@ class Epochs(object):
epochs[idx] : Epochs
Return Epochs object with a subset of epochs (supports single
index and python style slicing)
Copy link
Member

Choose a reason for hiding this comment

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

Actually it also supports numpy style indexing for the first dimension (events), doesn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by numpy style? If you mean fancy indexing, yes.

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

wrote:

In mne/epochs.py:

@@ -148,6 +165,13 @@ class Epochs(object):
epochs[idx] : Epochs
Return Epochs object with a subset of epochs (supports single
index and python style slicing)

Actually it also supports numpy style indexing for the first dimension
(events), doesn't it?


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

Copy link
Member

Choose a reason for hiding this comment

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

yes, indexing with an array of ints or booleans


For subset selection using categorial labels:

epochs['name'] : Epochs
Return Epochs object with a subset of epochs corresponding to an
experimental condition as specified by event_id.

"""
@verbose
def __init__(self, raw, events, event_id, tmin, tmax, baseline=(None, 0),
Expand All @@ -159,10 +183,21 @@ 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('Event IDs must be of type integer')
if not all([isinstance(k, str) for k in event_id]):
raise ValueError('Event names must be of type str')
self.event_id = event_id
elif isinstance(event_id, int):
self.event_id = {str(event_id): event_id}
elif event_id is None:
self.event_id = dict((str(e), e) for e in np.unique(events[:, 2]))
else:
raise ValueError('event_id must be dict or int.')
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 +244,9 @@ def __init__(self, raw, events, event_id, tmin, tmax, baseline=(None, 0),

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

n_events = len(self.events)

Expand Down Expand Up @@ -463,26 +497,47 @@ 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])
if self.preload:
data = self._data
del self._data

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 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):
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_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)
Copy link
Member

Choose a reason for hiding this comment

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

copying looks good to me now.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


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 +547,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 +566,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_id:
ids = self.events[self.events[0:, 2] == self.event_id[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 @@ -610,7 +672,7 @@ def crop(self, tmin=None, tmax=None, copy=False):
tmask = (self.times >= tmin) & (self.times <= tmax)
tidx = np.where(tmask)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the copy? Epochs never modifies the raw object, so this seems unnecessary and inflates memory consumption. Especially when raw has been preloaded and we do e.g. epochs['left_visual'], the whole Raw object will be copied (see @christianmbrodbeck comment above)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, a swe wrote the Epochs.copy I wasn't sure whether there might be cases
when the epochs modifies the raw...
What about Epochs.copy(copy_raw=False) as default?

Copy link
Member

Choose a reason for hiding this comment

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

I agree maybe we can live without the self.raw.copy()

That lead to bugs with raw gets modified between 2 sets of epochs but that can be considered bad practice

Copy link
Member

Choose a reason for hiding this comment

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

why not copy_raw argument but it will be unaccessible but the user in the getitem

Copy link
Member Author

Choose a reason for hiding this comment

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

additional copy_raw attribute then for this case?

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

wrote:

In mne/epochs.py:

@@ -610,7 +663,7 @@ def crop(self, tmin=None, tmax=None, copy=False):
tmask = (self.times >= tmin) & (self.times <= tmax)
tidx = np.where(tmask)[0]

why not copy_raw argument but it will be unaccessible but the user in the
getitem


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

Copy link
Contributor

Choose a reason for hiding this comment

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

But Epochs doesn't do anything to Raw, it just reads from it. So never copying it should be perfectly fine..

The danger with introducing too many switches like this is that there is an increasing number of combinations that can result in a failure and testing every one of them becomes a combinatorial nightmare.

Copy link
Member

Choose a reason for hiding this comment

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

old man wisdom :)

I agree -1 on copy_raw. It's not necessary as not exposed anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

On Thu, Dec 6, 2012 at 3:47 PM, Martin Luessi notifications@github.comwrote:

In mne/epochs.py:

@@ -610,7 +663,7 @@ def crop(self, tmin=None, tmax=None, copy=False):
tmask = (self.times >= tmin) & (self.times <= tmax)
tidx = np.where(tmask)[0]

But Epochs doesn't do anything to Raw, it just reads from it. So never
copying it should be perfectly fine..

Ok, convinced.

The danger with introducing too many switches like this is that there is
an increasing number of combinations that can result in a failure and
testing every one of them becomes a combinatorial nightmare.

Yes, totally agree, before switching to Haskell let's keep up good
programming practice ;-)

Wo we will write an epochs.copy that does not copy the raw. Is this what we
converge on?


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

Copy link
Member

Choose a reason for hiding this comment

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

That lead to bugs with raw gets modified between 2 sets of epochs but that can be considered bad practice

If you want to warn users about to do that, the raw object could have a _has_dependents attribute that is True if an Epochs object has already been created from it, and could issue a warning when modifying the raw data?


this_epochs = self if not copy else cp.deepcopy(self)
this_epochs = self if not copy else self.copy()
this_epochs.tmin = this_epochs.times[tidx[0]]
this_epochs.tmax = this_epochs.times[tidx[-1]]
this_epochs.times = this_epochs.times[tmask]
Expand Down Expand Up @@ -643,9 +705,12 @@ def resample(self, sfreq, npad=100, window='boxcar'):
def copy(self):
""" Return copy of Epochs instance
"""
raw = self.raw.copy()
raw = self.raw
del self.raw
new = deepcopy(self)
self.raw = raw
new.raw = raw

return new

def save(self, fname):
Expand Down Expand Up @@ -704,7 +769,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 Expand Up @@ -1011,7 +1075,7 @@ def read_epochs(fname, proj=True, verbose=None):
epochs._projector, epochs.info = setup_proj(info)
epochs.ch_names = info['ch_names']
epochs.baseline = baseline
epochs.event_id = int(np.unique(events[:, 2]))
epochs.event_id = dict((str(e), e) for e in np.unique(events[:, 2]))
fid.close()

return epochs
Expand All @@ -1038,7 +1102,7 @@ def bootstrap(epochs, random_state=None):
'in the constructor.')

rng = check_random_state(random_state)
epochs_bootstrap = cp.deepcopy(epochs)
epochs_bootstrap = epochs.copy()
n_events = len(epochs_bootstrap.events)
idx = rng.randint(0, n_events, n_events)
epochs_bootstrap = epochs_bootstrap[idx]
Expand Down
18 changes: 17 additions & 1 deletion mne/tests/test_epochs.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
# License: BSD (3-clause)

import os.path as op
from nose.tools import assert_true, assert_equal
from nose.tools import assert_true, assert_equal, assert_raises
from numpy.testing import assert_array_equal, assert_array_almost_equal
import numpy as np
import copy as cp
Expand Down Expand Up @@ -381,3 +381,19 @@ def test_epoch_eq():
assert_true(epochs_1.events.shape[0] != epochs_2.events.shape[0])
equalize_epoch_counts(epochs_1, epochs_2)
assert_true(epochs_1.events.shape[0] == epochs_2.events.shape[0])


def test_access_by_name():
assert_raises(ValueError, Epochs, raw, events, {1: 42, 2: 42}, tmin,
tmax, picks=picks)
assert_raises(ValueError, Epochs, raw, events, {'a': 'spam', 2: 'eggs'},
tmin, tmax, picks=picks)
assert_raises(ValueError, Epochs, raw, events, {'a': 'spam', 2: 'eggs'},
tmin, tmax, picks=picks)
assert_raises(ValueError, Epochs, raw, events, 'foo', tmin, tmax, picks=picks)
epochs = Epochs(raw, events, {'a': 1, 'b': 2}, tmin, tmax, picks=picks)
assert_raises(KeyError, epochs.__getitem__, 'bar')

data = epochs['a'].get_data()
event_a = events[events[:, 2] == 1]
assert_true(len(data) == len(event_a))