[MRG] Add support for indexing/slicing Annotations objects #5800

massich · 2018-12-17T20:26:23Z

Based on python 3 doc indexing with
the wrong type should raise TypeError not IndexError. Then the entire function gets much more simpler:

def __getitem__(self, key):
    """Propagate indexing and slicing to the underlying numpy structure."""
    return (self.onset[key], self.duration[key], self.description[key])

About the current solution:

(pros) I like the idea that we are raising the right
(cons) the code has 5 branches

agramfort · 2018-12-17T20:28:24Z

mne/tests/test_annotations.py

+    """Test indexing Annotations."""
+    NUM_ANNOT = 5
+    EXPECTED_ONSETS = EXPECTED_DURATIONS = [_ for _ in range(NUM_ANNOT)]
+    EXPECTED_DESCS = [_.__repr__() for _ in range(NUM_ANNOT)]


I would not use _ for a param you actually use

doc/whats_new.rst

massich · 2018-12-17T20:48:04Z

based on #5795 (comment) maybe we should return a copy. But I'm not sure.

But I guess that @larsoner was trying to avoid this?

my_wrong_recorded_annotatoins = [d.startswith('foo') for d in raw.annotations.description]
onsets, _, _ = raw.annotations[my_wrong_recorded_annotatoins]
onsets += 10

If you want to do that you should do raw.annotations.onset[my_wrong_recorded_annotations] += 10

agramfort · 2018-12-17T20:53:44Z

+1 to return a copy

…

larsoner · 2018-12-17T20:56:41Z

Yes in MNE we (should, at least) always return a copy with indexing operations on our objects. This makes us different from NumPy, which has inplace and copy rules.

codecov · 2018-12-19T13:00:08Z

Codecov Report

Merging #5800 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5800      +/-   ##
==========================================
+ Coverage   88.57%   88.58%   +0.01%     
==========================================
  Files         369      369              
  Lines       68934    69004      +70     
  Branches    11614    11631      +17     
==========================================
+ Hits        61055    61126      +71     
+ Misses       5027     5025       -2     
- Partials     2852     2853       +1

agramfort · 2018-12-19T13:03:50Z

tutorials/plot_object_annotations.py

+# with the sliced elements.
+#
+# See the following examples and usages:
+plt.close('all')


why matplotlib here?

This should go.

agramfort · 2018-12-19T13:07:31Z

tutorials/plot_object_annotations.py

+start, stop, step = (0, None, 2)
+every_other_annotation = slice(start, stop, step)
+for onset, duration, desc in zip(*annot[every_other_annotation]):
+    print('onset={0} duration={1} desc={2}'.format(onset, duration, desc))


I would try to dumb it down. Here you use advanced string formatting, for loops etc. I would just do:

annotations[:3] # will return a new Annotations formed by the first 3 annotations[2] # will return a new Annotations restricted to the 3rd annotation.

and I would point to python indexing doc as it behaves likes for str or lists etc.

Thats exactly what does not wat I expected from the tests.

agramfort · 2018-12-19T13:07:50Z

tutorials/plot_object_annotations.py

 =========================================================================

-Events and :class:`~mne.Annotations` are quite similar.
+:term:`Events <events>` and :term:`annotations` are quite similar.


term links work?

massich · 2018-12-19T13:35:14Z

I confused __getitem__ and __iter__ behavior.

agramfort · 2018-12-21T09:34:46Z

mne/annotations.py

+                out = Annotations(onset=[self.onset[key]],
+                                  duration=[self.duration[key]],
+                                  description=[self.description[key]],
+                                  orig_time=self.orig_time)


I would return a tuple like

(self.onset[key], self.duration[key], self.description[key])

this would be consistent with the iterator.

or it could be a dict with keys onset, duration, description like when you index a pandas dataframe...

thoughts @jona-sassenhagen ?

After talking to @agramfort offline he convinced me to return a dictionary not an Annotations when indexing with a single integer.

It has the advantage that if we ever want to extend the annotations, with more fields we won't break people's code.

What do you think?

Why don't we return an Annotations object for the iterator and for slicing/indexing operations? It has the same extend-ability because whatever we add will be attributes of the Annotations class immediately.

This is also basically what we do with the Epochs class, so it's more consistent with that, too.

@mne-tools/contributors ?

So the proposals are to return:

annotations object

tuple

dict

?

I haven't worked with annotations enough to have an opinion ...

Yep, so concretely:

# annot for a in raw.annotations: onset, duration, desc = a.onset[0], a.duration[0], a.desc[0] ... # tuple for onset, duration, desc in raw.annotations: ... # dict for a in raw.annotations: onset, duration, desc = a['onset'], a['duration'], a['desc'] ...

The only advantage of an dict approach over iterating over annotations objects I see is that it avoids the [0], but this does not seem worth introducing more API inconsistency to me.

returning dict is more consistent with dataframe behavior and will not break if you start adding a channel name for annotations

The annot will also not break if you start adding a channel name for annotations.

So the question is, do we value internal package consistency (Annotations iterating like Epochs) or consistency with what Pandas does (Annotations iterating like pandas) here?

if you have an iterable container c in Python if you do:

for k in range(len(c)): print(c[k])

it is equivalent to:

for k in c: print(k)

this works for lists, strs, arrays, etc.

My decision to have epochs[k] return epochs with nave=1 is I think an historical error yet quite convenient.
To get this you should have needed to do epochs[k: k + 1]
So this is not a pandas thing.

What is a pandas thing is the fact that annot[k] would return a dict and not a tuple. As when you do

s = df.iloc[k]

you get s as Series whose semantic matches a dictionary as the column names becomes the index.

does it make any sense?

I think the argument you are really trying to make has to do with how conceptualize iteration over an N-dimensional object: is what you get in the loop (N-1)D (like arrays, lists, tuples, pandas, etc.) or ND (like epochs). I can see why this would be useful, even if it's less like what we usually do, so I can live with it.

(FWIW, I don't quite see why the iterable argument helps, since it actually holds already for the epochs object (epochs[k] == e for k, e in enumerate(epochs)) and would hold for the annotations API (annot[k] == a for k, a in annotations). It seems to actually favor "iter yields Annotations" if __getitem__ always returns Annotations; in order for the relationship to hold for "iter yields dict", we'd need Annotations | dict to be returned by __getitem__, depending on whether the result is slice vs int...)

agramfort · 2018-12-25T13:39:15Z

it does not hold for epochs class. if you iterate over epochs you get arrays but when you do epochs[k] you get an epochs object

larsoner · 2018-12-25T16:38:40Z

Ahh true I thought it was the other way but did not check.

FWIW the iter/index equivalence argument still seems to go against the proposal, though, right?

agramfort · 2018-12-27T08:49:04Z

Ahh true I thought it was the other way but did not check. FWIW the iter/index equivalence argument still send to go against the proposal, though, right?

I don't think so. I propose to get a dictionary when you iter or access k'th element.

larsoner · 2018-12-27T13:07:54Z

So in getitem, int gives dict and slice gives Annotations object?

larsoner · 2018-12-27T13:10:34Z

Or I guess we could have slice give a dict with values that are 2D ndarray. Then if you want Annotations it's just a reconstruction with double star away

agramfort · 2018-12-27T13:16:41Z

slice gets you Annotations and int gets you a dict clear?

…

larsoner · 2018-12-27T13:41:56Z

I can live with it

agramfort · 2019-01-09T17:41:14Z

mne/annotations.py

        else:
-            return out
+            key = list(key) if isinstance(key, tuple) else key
+            return Annotations(onset=self.onset[key],


if key is a slice you should force the copy of onset etc.

Doesn't it take a copy by calling the constructor?
I've to check that.

agramfort · 2019-01-09T17:58:25Z

no copy when it's a slice

…

massich · 2019-01-10T10:13:00Z

I guess I'm testing something wrong, because I don't see the effect of the copy.

    NUM_ANNOT = 5
    EXPECTED_ONSETS = EXPECTED_DURATIONS = [x for x in range(NUM_ANNOT)]
    EXPECTED_DESCS = [x.__repr__() for x in range(NUM_ANNOT)]

    annot = Annotations(onset=EXPECTED_ONSETS,
                        duration=EXPECTED_DURATIONS,
                        description=EXPECTED_DESCS,
                        orig_time=None)

    print((id(annot.onset),
           id(annot[:1].onset),
           id(annot.onset) == id(annot[:1].onset)))
    print((id(annot.onset[0]),
           id(annot[:1].onset[0]),
           id(annot.onset[0]) == id(annot[:1].onset[0])))
    print(annot.onset[0])    NUM_ANNOT = 5
    EXPECTED_ONSETS = EXPECTED_DURATIONS = [x for x in range(NUM_ANNOT)]
    EXPECTED_DESCS = [x.__repr__() for x in range(NUM_ANNOT)]

    annot = Annotations(onset=EXPECTED_ONSETS,
                        duration=EXPECTED_DURATIONS,
                        description=EXPECTED_DESCS,
                        orig_time=None)

    print((id(annot.onset),
           id(annot[:1].onset),
           id(annot.onset) == id(annot[:1].onset)))
    print((id(annot.onset[0]),
           id(annot[:1].onset[0]),
           id(annot.onset[0]) == id(annot[:1].onset[0])))
    print(annot.onset[0])
    print((id(annot.onset[0]),
           id(annot[:1].onset[0]),
           id(annot.onset[0]) == id(annot[:1].onset[0])))
    annot[:1].onset[0] = 42
    print(annot.onset[0])
    print((id(annot.onset[0]),
           id(annot[:1].onset[0]),
           id(annot.onset[0]) == id(annot[:1].onset[0])))
    print((id(annot.onset[0]),
           id(annot[:1].onset[0]),
           id(annot.onset[0]) == id(annot[:1].onset[0])))
    annot[:1].onset[0] = 42
    print(annot.onset[0])
    print((id(annot.onset[0]),
           id(annot[:1].onset[0]),
           id(annot.onset[0]) == id(annot[:1].onset[0])))

the result is both the same using copy or not.

(139650719663168, 139650338620800, False)
(139650385747040, 139650385747040, True)
0.0
(139650385747040, 139650385747040, True)
0.0
(139650385747040, 139650385747040, True)

The returned list is a different one, but the elements inside are the same. But the change has no effect. I guess I'm doing something wrong. I just added the copy() to make sure.

massich · 2019-01-10T10:18:59Z

which is different than this:

xx = np.array(range(3))
xx[:1][0]=42
print(xx)

[42  1  2]

massich · 2019-01-10T10:24:33Z

I guess that what I'm saying is that I could not figure out how to write a test that actually breaks if .copy() is not there and passes otherwise.

larsoner · 2019-01-10T16:40:04Z

I could not figure out how to write a test that actually breaks if .copy() is not there and passes otherwise.

That's because there is an implicit copy in the np.array(...) calls in the constructor:

https://github.com/mne-tools/mne-python/blob/master/mne/annotations.py#L157

So you shouldn't need to do any .copy() calls if the variables are passed to Annotations(...)

larsoner · 2019-01-10T16:40:34Z

mne/annotations.py

+            key = list(key) if isinstance(key, tuple) else key
+            return Annotations(onset=self.onset[key].copy(),
+                               duration=self.duration[key].copy(),
+                               description=self.description[key].copy(),


... so no need for any of these copy calls, because a copy is made in Annotations.__init__

agramfort · 2019-01-10T17:45:43Z

good catch !

massich · 2019-01-10T21:38:50Z

I did not see this one.

self.onset = np.array(onset, dtype=float)

Great.

massich · 2019-01-10T21:41:00Z

This still remains though

(139650719663168, 139650338620800, False)
(139650385747040, 139650385747040, True)
0.0
(139650385747040, 139650385747040, True)
0.0
(139650385747040, 139650385747040, True)

The vectors do have different id (they are indeed a copy) but the first element of both vectors share the id. Anyway.. I'll let it be. this outsmarts me.

doc/whats_new.rst

mne/tests/test_annotations.py

larsoner

@cbrnr feel free to merge if you are happy

cbrnr · 2019-01-11T20:14:50Z

Thanks @massich!

agramfort reviewed Dec 17, 2018

View reviewed changes

doc/whats_new.rst Outdated Show resolved Hide resolved

massich force-pushed the iter_annotations branch from 421f85a to a41d96a Compare December 17, 2018 20:30

agramfort reviewed Dec 19, 2018

View reviewed changes

massich changed the title ~~[MRG] Add support for indexing/slicing Annotations objects~~ [WIP] Add support for indexing/slicing Annotations objects Dec 19, 2018

massich force-pushed the iter_annotations branch from a8c7bdc to 6e29f94 Compare December 20, 2018 11:14

Joan Massich added 6 commits December 20, 2018 20:29

Add support for indexing and slicing Annotations

753eba1

FIX: Avoid colateral effects

8adaf41

[wip] documents slicing

c7ee71b

xx

0e8a05a

remove plt close

7a36be1

more on slicning / iter

da741a6

massich force-pushed the iter_annotations branch from 6e29f94 to da741a6 Compare December 20, 2018 21:50

fix

d53d48b

agramfort reviewed Dec 21, 2018

View reviewed changes

Merge branch 'master' into iter_annotations

9b52e9c

Joan Massich added 5 commits January 9, 2019 17:13

TST: break slicing

fdfdbf4

fix tuple

6d681b5

update tests

5ccafd3

typo

7037ac1

cosmit

884340e

agramfort reviewed Jan 9, 2019

View reviewed changes

Joan Massich added 2 commits January 9, 2019 19:07

this should break if slicing returns a view and not a copy

d70125a

return copy on slice

5b09ff3

massich changed the title ~~[WIP] Add support for indexing/slicing Annotations objects~~ [MRG] Add support for indexing/slicing Annotations objects Jan 10, 2019

nitpick

7b701ce

larsoner reviewed Jan 10, 2019

View reviewed changes

remove copy

df9a952

cbrnr reviewed Jan 11, 2019

View reviewed changes

doc/whats_new.rst Outdated Show resolved Hide resolved

cbrnr reviewed Jan 11, 2019

View reviewed changes

mne/tests/test_annotations.py Outdated Show resolved Hide resolved

Joan Massich added 3 commits January 11, 2019 14:52

add iterating to whats new

14b72b5

better test

ef109e4

remove the test

1c71128

agramfort approved these changes Jan 11, 2019

View reviewed changes

larsoner approved these changes Jan 11, 2019

View reviewed changes

cbrnr merged commit e2c538b into mne-tools:master Jan 11, 2019

cbrnr deleted the iter_annotations branch January 11, 2019 20:14

Uh oh!

[MRG] Add support for indexing/slicing Annotations objects #5800

[MRG] Add support for indexing/slicing Annotations objects #5800

Uh oh!

Conversation

massich commented Dec 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

massich commented Dec 17, 2018

Uh oh!

agramfort commented Dec 17, 2018 via email

Uh oh!

larsoner commented Dec 17, 2018

Uh oh!

codecov bot commented Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

massich commented Dec 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Dec 25, 2018 via email

Uh oh!

larsoner commented Dec 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort commented Dec 27, 2018 via email

Uh oh!

larsoner commented Dec 27, 2018

Uh oh!

larsoner commented Dec 27, 2018

Uh oh!

agramfort commented Dec 27, 2018 via email

Uh oh!

larsoner commented Dec 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Jan 9, 2019 via email

Uh oh!

massich commented Jan 10, 2019

Uh oh!

massich commented Jan 10, 2019

codecov bot commented Dec 19, 2018 •

edited

Loading

larsoner Dec 21, 2018 •

edited

Loading

larsoner commented Dec 25, 2018 •

edited

Loading