-
Notifications
You must be signed in to change notification settings - Fork 261
add to_epoch method in Event class: Transform Events into an array of Epoch #404
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
Conversation
neo/core/event.py
Outdated
print("self.label : " + str(self.labels)) | ||
i = 0 | ||
duration = [] | ||
for t in self.times: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable t is never used. Maybe it would be better to use 'for i in range(len(self.times)):'?
neo/core/event.py
Outdated
if i==0 : | ||
duration.append(self.times[i]) | ||
else : | ||
duration.append(self.times[i] - self.times[i-1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a better performace you could use np.diff(self.times) to calculate the durations instead of looping.
It looks as if you are generating epochs with starting at the times of the events, but having a duration corresponding to the time difference since the previous event. I would have rather expected epochs spanning the time between the events. Is this intended and under which conditions is it useful? |
@JuliaSprenger yes, I think the indexing is off by one. We discussed in Lausanne the question of whether to make this a method or a helper function. There are arguments for both. In the end, I think having it as a method gives better discoverability - it is attached to the Event class, which I think makes it easier to find if you already have an Event object, compared to having to search through a potentially long list of helper functions. Of course we could have both, where the helper function calls the method or vice versa. @jonathanduperrier I think the top priority now is to add some unit tests (in the file |
This looks also very strange to me. I add in mind that to_epoch would be: Epoch(time=event.times[:-1], durations=np.diff(event.times), labels=event.labels[:-1]) Here, in thisimplementation, the duration is with the previsous event. I don't get the use case. |
I agree with @samuelgarcia, this should be the default behaviour of the function. However, do you think it would make sense to introduce the option to provide durations in case you don't want to cover the whole recording time with the epochs but only specific time windows after the events? def to_epoch(durations=None):
if durations is None:
times = self.times[:-1]
durations = np.diff(self.times)[:-1]
labels = self.labels[:-1]
else:
times = self.times
labels = self.labels
return Epoch(time=times, durations=durations, labels=labels) |
Hello @jonathanduperrier! Thanks for updating the PR.
Comment last updated on March 02, 2018 at 09:26 Hours UTC |
.travis.yml
Outdated
# command to run tests, e.g. python setup.py test | ||
script: | ||
nosetests --with-coverage --cover-package=neo | ||
assert_array_equal(epoch.magnitude, np.array([7.0, 11.0, 22.0])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you meant to put this line in test_epoch.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I meant test_event.py
I propose to postpone thie PR for 0.7. |
Since this now has conflicts, I have updated @jonathanduperrier's code and made a new PR: #586 |
add to_epoch method in Event class: Transform Events into an array of Epoch