Implement __getitem__ for Epoch

[apdavison]

so that durations and labels are correctly sliced. Fixes #413

[pep8speaks]

Hello @apdavison! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on March 01, 2018 at 11:30 Hours UTC

[coveralls]

Coverage increased (+0.05%) to 49.221% when pulling a51ab8b on apdavison:issue413 into 6fd9f49 on NeuralEnsemble:master.

[coveralls]

Coverage increased (+0.05%) to 48.849% when pulling 5a91066 on apdavison:issue413 into 6b6c7ef on NeuralEnsemble:master.

[samuelgarcia]

Hi andrew. Could you fix conflict in the file. So it would trigered a new test on circleci ?
So we could merge.

[muellerbjoern]

I have a question about this, because I have been working on SpikeTrain.__getitem__ for the last days. It's rather general, but I noticed this here.

I noticed here that what you wrote works in Epoch for things like epoch[1], while it would raise an error in SpikeTrain, because when you create the new Epoch in __getitem__, times is a quantity scalar, NOT an array or Epoch. Creating a SpikeTrain like this in __getitem__ via SpikeTrain(times=super(...).__getitem, ...) would raise an error here saying it's testing the length of an unsized object. Is it supposed to be okay that epochs are created with a scalar quantity instead of a quantity array? And if that is the case, shouldn't it be converted to an array then in order to ensure consistency?

A little example of what happens:

epoch = neo.Epoch([1,2,3]*pq.s)
ep1 = epoch[1] # ep1 is now an Epoch with times=2*pq.s, not [2]*pq.s or array([2]*pq.s)

This is because the following happens in __getitem__:
times = super(...).__getitem__(1)
Times now becomes a scalar quantity, because numpy __getitem__ returns a scalar instead of an array when called with int index. From this pq.Quantity.getitem creates a quantity, because it is not yet a quantity.

obj = Epoch(times=times, ...)
This is not a problem, because there is no test about whether times is an array or not
...
return obj # This returns obj, with times=2*pq.s

Should it be like this or should a test for this be introduced?

Apart from that I'd like to know if any of you has deeper knowledge of __getitem__ because I noticed together with @JuliaSprenger that there sometimes a scalar quantity is returned instead of a SpikeTrain object. I've been trying to change this but it fails. Trying to copy what you did, I noticed that this works for Epoch, but not for SpikeTrain.

Again some code to illustrate what I mean:

What currently happens:
st1 = new SpikeTrain(times=[1,2,3]*pq.s, ...)
testobj = st1[1]
testobj is now a (scalar) quantity object, not a SpikeTrain
print(testobject) # This prints 2 s

new_st = st1[0:1]
new_st is a SpikeTrain, because numpy returns a SpikeTrain array
print(new_st) # This prints [1] s

If one would do the same as you did in Epoch for SpikeTrain, it would raise an error
st2 = st1[1] # This would raise an error

What happens SpikeTrain __getitem__:

times = super(...).__getitem__(1)
times is now a scalar quantity just like in Epoch

obj = SpikeTrain(times=times, ...)
This will raise an error in SpikeTrain.__new__ (line 220), because times is a scalar, and thus an unsized object: TypeError: len() of unsized object

On the other hand, calling things like st1[0:1] will work just fine, because numpy then returns an array (or a subclass, which is SpikeTrain in this case)

I then tried creating a new SpikeTrain if I get a quantity by creating a list with a single entry:

if not isinstance(obj, SpikeTrain):
obj = SpikeTrain(times=obj.magnitude, t_stop=self.t_stop, units=self.units) # Error happens here

This works for most cases, but it fails whenever calling sorted(np.concatenate((st1, st2))), because apparently in concatenate a kind of 'intermediate' SpikeTrain is generated that doesn't contain any attributes.
The error I get then is that SpikeTrain object [referring to self here] has no attribute t_stop.
If anybody knows more about this stuff, I'd really appreciate your help.

[muellerbjoern]

I kept working on this problem and noticed that returning a SpikeTrain when st[int] is called would create more problems than it solves. So it seems to be fine that a scalar Quantity object is returned.

But in my opinion this should be consistent across all neo objects, which means that also here in Epoch.getitem an Epoch should be returned only, when ep[slice] was called, not when ep[int] was called. Do you agree with this @apdavison @samuelgarcia?
This would also simplify the code a bit here, because no new Epoch would have to be created but instead you would only have to check if an Epoch was returned from super(...).getitem() and if that is the case, slice durations correctly, like it is done in SpikeTrain.getitem.

On a more general level, as I wrote in the previous comment, I want to ask if it makes sense to allow Epochs to be created with times being a scalar instead of an array. In class SpikeTrain there are checks that fail if times is not a list or an array, whereas in Epoch nothing similar happens. If times is a scalar, then ep.times is simply a scalar quantity. Are there reasons for this or should checks be implemented to make sure it's consistent? And if it should be allowed, wouldn't it make sense to wrap it in an array then?
Apart from that it's also not assured that ep.labels and ep.durations are the same length as ep.times. I think this should also be checked in order to ensure consistency.

[apdavison]

I've rebased this onto master to remove the conflicts.

I suggest creating a separate issue for the inconsistencies noted by @bjoern1001001 with respect to spike trains, and a separate PR to fix them.

[samuelgarcia]

@bjoern1001001 : could you create a sperate issue for you comment. So I could merge and close this without forgeting the issus on getitem and scalar inside neo.

[samuelgarcia]

I have opened an issue #490 that refer this PR so I merge.