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

Epoch concatenation #120

Closed
larsoner opened this issue Sep 26, 2012 · 12 comments
Closed

Epoch concatenation #120

larsoner opened this issue Sep 26, 2012 · 12 comments

Comments

@larsoner
Copy link
Member

Let me know if this is implemented and I already missed it, but I'd like to implement a method for concatenating Epoch objects. We typically use 6-9 runs for our experiments, and it's useful to be able to treat these as being equal at some point. To concatenate epochs, I think all I'd need to do is check to make sure these properties were equal across epochs before concatenating data:

  1. info['chs']
  2. info['ch_names']
  3. info['nchan']
  4. picks
  5. proj
  6. info['projs']
  7. times

If any of these were not equal across epoch objects, an error would be thrown stating that only objects with equivalent values could be concatenated.

Then the following would need to be concatenated appropriately:

  1. events
  2. _data

One tricky part is what to do with data preloading. An easy (lazy) solution is to require preloading, so that the above concatenation of _data will work. The other option is to implement multiple-source-file support in _get_data_from_disk(), but maybe as a first pass this could be disabled and left to a future improvement?

@agramfort
Copy link
Member

I agree with all what you said. Basically you have 3 options:

  • concat Raw instances and work at the raw level
  • concat Epochs and return a new Epochs
  • concat Epochs into a new object on with we can iterate and call average
    and get_data. The concat magic would happen when accessing the data

wdyt?

@larsoner
Copy link
Member Author

I like options 1 and 2 potentially better than option 3 (new object), because part of the motivation behind this is to continue to have access to all enhancements brought to the Epochs objects. That is, unless the new object (MultiEpochs or whatever) would inherit the Epochs class and just overload the average() and get_data() functions (and in this sense, be transparent to the user as otherwise being of the Epochs class)?

@agramfort
Copy link
Member

I like options 1 and 2 potentially better than option 3 (new object),
because part of the motivation behind this is to continue to have access to
all enhancements brought to the Epochs objects. That is, unless the new
object (MultiEpochs or whatever) would inherit the Epochs class and just
overload the average() and get_data() functions (and in this sense, be
transparent to the user as otherwise being of the Epochs class)?

yes that would work. I'd say I'd have to try to implement it and use
it to see if it actually flies...

do not hesitate to give it a try as you clearly understood the
implications of those changes.

@larsoner
Copy link
Member Author

I'll probably give the overloading a shot first, as this should be the easiest way not to require people to preload all their data.

@dengemann
Copy link
Member

+1 for this.
My guess would be that 2) is the most clear / straight-forward option.
One option would be to include a .concatenate method for the epochs,
another one could be to do to override the add operator.
I personally hold a slight preference for a method or a function, as this
seems to involve less magic (although theres definitely something charming
to 'epochs_a + epochs_b').

2012/9/26 Eric89GXL notifications@github.com

I'll probably give the overloading a shot first, as this should be the
easiest way not to require people to preload all their data.


Reply to this email directly or view it on GitHubhttps://github.com//issues/120#issuecomment-8897627.

@larsoner
Copy link
Member Author

Well, I just spent an embarrassing amount of time on this today to figure out that there's a problem with creating a new class (e.g., MultiClass) that inherits and extends the Epoch class. We'd either need to use a bunch of deepcopy()'s, or warn users against using it in certain ways.

In any case, I think it'll be cleaner to refactor the current Epochs class to use either 1) one raw input and one list of events (current behavior), or 2) a list of raws with a list of events (extended, new behavior). This will not only be easier to use than making a new class, but it easily retains backward compatibility, makes for a natural extension with a concatenate() call, and doesn't require people learning a new class. I'm going to work on that tomorrow.

@dengemann
Copy link
Member

On 27.09.2012, at 01:31, Eric89GXL notifications@github.com wrote:

Well, I just spent an embarrassing amount of time on this today to figure out that there's a problem with creating a new class (e.g., MultiClass) that inherits and extends the Epoch class. We'd either need to use a bunch of deepcopy()'s, or warn users against using it in certain ways.

Ok, this does not sound ideal.. :-(

In any case, I think it'll be cleaner to refactor the current Epochs class to use either 1) one raw input and one list of events (current behavior), or 2) a list of raws with a list of events (extended, new behavior). This will not only be easier to use than making a new class, but it easily retains backward compatibility, makes for a natural extension with a concatenate() call, and doesn't require people learning a new class. I'm going to work on that tomorrow.

Sounds very good to me. And certainly getting into mixing classes and deepcopying nested structures is not exactly desirable.. (at least i don't like it)
Just free-wheeling: Maybe another take could be the use of class methods with alternative constructors and .new vs from_raws methods or so.
But probably it would be the most clean thing, as you say, to take the constructor as is and make its behavior contingent upon its input arguments, that is raw object vs list of raw objects.


Reply to this email directly or view it on GitHub.

@agramfort
Copy link
Member

Well, I just spent an embarrassing amount of time on this today to figure
out that there's a problem with creating a new class (e.g., MultiClass) that
inherits and extends the Epoch class. We'd either need to use a bunch of
deepcopy()'s, or warn users against using it in certain ways.

that's what I was afraid of.

In any case, I think it'll be cleaner to refactor the current Epochs class
to use either 1) one raw input and one list of events (current behavior), or
2) a list of raws with a list of events (extended, new behavior). This will
not only be easier to use than making a new class, but it easily retains
backward compatibility, makes for a natural extension with a concatenate()
call, and doesn't require people learning a new class. I'm going to work on
that tomorrow.

sounds very reasonable and cleaner. What I've been willing to do too is to
instantiate a Raw with a list of fif files in case the run is too long.

A

@dengemann
Copy link
Member

On 27.09.2012, at 09:24, Alexandre Gramfort notifications@github.com wrote:

Well, I just spent an embarrassing amount of time on this today to figure
out that there's a problem with creating a new class (e.g., MultiClass) that
inherits and extends the Epoch class. We'd either need to use a bunch of
deepcopy()'s, or warn users against using it in certain ways.

that's what I was afraid of.

In any case, I think it'll be cleaner to refactor the current Epochs class
to use either 1) one raw input and one list of events (current behavior), or
2) a list of raws with a list of events (extended, new behavior). This will
not only be easier to use than making a new class, but it easily retains
backward compatibility, makes for a natural extension with a concatenate()
call, and doesn't require people learning a new class. I'm going to work on
that tomorrow.

sounds very reasonable and cleaner. What I've been willing to do too is to
instantiate a Raw with a list of fif files in case the run is too long.

A

... so one could search through all of them for events and instantiate an epochs object with more or less the existing code...

Bu how would the raw object then look like, e.g. The info structure?


Reply to this email directly or view it on GitHub.

@agramfort
Copy link
Member

Bu how would the raw object then look like, e.g. The info structure?

the info should be exactly the same. That's valid if it's actually the same run.

@dengemann
Copy link
Member

Then maybe this should be the way to go. Maybe other use cases will emerge and then we're best off with a Raw object that handles multiple fif files if desired.

On 27.09.2012, at 09:51, Alexandre Gramfort notifications@github.com wrote:

Bu how would the raw object then look like, e.g. The info structure?

the info should be exactly the same. That's valid if it's actually the same run.

Reply to this email directly or view it on GitHub.

@agramfort
Copy link
Member

Then maybe this should be the way to go. Maybe other use cases will emerge
and then we're best off with a Raw object that handles multiple fif files if
desired.

I think we need both as the multiple fif input to raw should assume
that first_samp or the next is last_samp from the previous.

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

No branches or pull requests

3 participants