[MRG] Handle events with non-unique event time samples #6688

sappelhoff · 2019-08-21T14:34:33Z

To Do

update event_id by removing a key-value-pair if the value is no longer in events after handle_duplicate_events
if event_repeated is "merge" but a duplicate actually has the same event code, switch to "drop" behavior
fix for 31d1eda: instead of updating events on every loop iteration, collect indices to delete and deleta all at once. --> see 323b61a
2nd fix for 31d1eda: "aud/vis" and "vis/aud" should map to the same thing --> see e72fcaa
updated whatsnew

sappelhoff · 2019-08-21T14:53:39Z

I am realizing that the '%s/%s' % (event_1_key, event_2_key) does not work, because in the mne events, we expect integers ... not strings.

Any advice @larsoner ?

larsoner · 2019-08-21T15:32:12Z

You need to

Come up with new event numbers for the events array, and
associate these properly with event_id dict

For example if in neither events nor event_id they use the integer 5 for anything, then you would make your events_id['x/y'] = 5 and set events[idx, 2] = 5

lgtm-com · 2019-08-21T15:57:12Z

This pull request introduces 2 alerts when merging c03b591 into bf5df34 - view on LGTM.com

new alerts:

2 for Unused local variable

mne/epochs.py

jasmainak · 2019-08-22T03:51:52Z

@sappelhoff you also need to update some tests

sappelhoff · 2019-08-22T14:02:10Z

Okay, I think this is a reasonable proposal now @jasmainak @larsoner @agramfort @massich

Before I start to implement testing, please let me know what you think.

Also: I need advise on what I need to edit if I want this parameter to be available in Epochs, EpochsArray, BaseEpochs, and so on.

mne/epochs.py

jasmainak · 2019-08-22T14:13:37Z

Please go ahead and write the tests. It will help us check the logic better.

codecov · 2019-08-22T14:52:36Z

Codecov Report

Merging #6688 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #6688      +/-   ##
==========================================
+ Coverage   89.48%   89.48%   +<.01%     
==========================================
  Files         420      420              
  Lines       75474    75596     +122     
  Branches    12369    12388      +19     
==========================================
+ Hits        67535    67647     +112     
- Misses       5134     5137       +3     
- Partials     2805     2812       +7

jasmainak · 2019-08-22T20:26:18Z

mne/tests/test_epochs.py

+    """Test handling of duplicate events."""
+    # A general test case
+    EVENT_ID = {'aud': 1, 'vis': 2}
+    EVENTS = np.array([[0, 0, 1], [0, 0, 2]])


why capital letters?

because these are my "constants" for the test

I would rename them EXPECTED_BLA and I would not use them on the input. ;)

mne/epochs.py

sappelhoff · 2019-08-22T21:13:08Z

Okay, tests are fully implemented now. Ready for another review :-)

massich · 2019-08-23T07:17:27Z

mne/epochs.py

@@ -183,6 +184,93 @@ def _save_split(epochs, fname, part_idx, n_parts, fmt):
    end_file(fid)


+def _handle_duplicate_events(events, event_id, event_repeated):


i like event_repeated_policy better.

_handle is a really ugly name.

what about def _apply_duplicate_events_policy(evetns, event_id, policy) or simply _duplicate_events_policy?

I'd rather not make the name longer. There is a tradeoff between explicit name clarity and usability. There is a good reason we don't go farther down the road of making it even more explicit like how_to_behave_when_events_occur_on_the_same_sample. I think event_repeated is clear enough as it is and more in line with how we tend to balance clarity with brevity.

I'll stay with event_repeated then, but will rename _handle_duplicate_events -> _handle_event_repeated

why don't you like "handle" @massich ? We are "handling" a situation after all?

mne/epochs.py

massich · 2019-08-23T07:59:35Z

mne/epochs.py

@@ -183,6 +184,93 @@ def _save_split(epochs, fname, part_idx, n_parts, fmt):
    end_file(fid)


+def _handle_duplicate_events(events, event_id, event_repeated):


_handle is a really ugly name.

what about def _apply_duplicate_events_policy(evetns, event_id, policy) or simply _duplicate_events_policy?

massich · 2019-08-23T08:01:45Z

mne/tests/test_epochs.py

+    """Test handling of duplicate events."""
+    # A general test case
+    EVENT_ID = {'aud': 1, 'vis': 2}
+    EVENTS = np.array([[0, 0, 1], [0, 0, 2]])


I would rename them EXPECTED_BLA and I would not use them on the input. ;)

mne/tests/test_epochs.py

massich · 2019-08-23T08:05:04Z

mne/tests/test_epochs.py

@@ -57,6 +59,66 @@
 rng = np.random.RandomState(42)


+def test_handle_duplicate_events():


can we parametrize some of it? (I've not checked)

mne/epochs.py

massich · 2019-08-23T08:09:46Z

all the code doing the job (the part inside merge) is really weird.

agramfort · 2019-08-23T08:17:51Z

@sappelhoff while trying the code I tried to the make the test more ambitious. It breaks the tests. Please have a look.

sappelhoff · 2019-08-23T10:20:20Z

all the code doing the job (the part inside merge) is really weird.

I am following what @agramfort and @jasmainak told me:

make it work
make it nice
make it fast

I feel like I can use your feedback well for step 2. @massich :-)

sappelhoff · 2019-08-25T14:30:07Z

Thanks for the reviews everybody. I addressed almost everything and the code is much better now (in the second iteration).

Unfortunately I won't have time to do a third iteration ... so it'd be great if somebody could take it from here!

lgtm-com · 2019-08-25T15:19:42Z

This pull request introduces 1 alert when merging 66fd60a into bf03e17 - view on LGTM.com

new alerts:

1 for Suspicious unused loop iteration variable

agramfort · 2019-08-25T15:44:40Z

Let us know when it's ready for reviews.

…

sappelhoff · 2019-08-25T15:48:23Z

Let us know when it's ready for reviews.

it's ready now - I just fixed a LGTM issue and a pydocstyle thing .... CIs should come back all green now.

mne/epochs.py

mne/tests/test_epochs.py

Co-Authored-By: Alexandre Gramfort <alexandre.gramfort@m4x.org>

agramfort

@jasmainak or @massich feel free to merge if happy

jasmainak · 2019-08-26T02:48:22Z

glad I didn't volunteer for this issue ;-) @massich I let you merge since I didn't follow the developments very closely

mne/epochs.py

massich · 2019-08-29T10:04:38Z

LGTM

@jona-sassenhagen

* add skeleton * show blocker * clarify docstr * add option 'drop' * find new event code and update event_id * dont reuse event codes that are in events * typo * continue work in private func * use event_id combination to make up new event_id * improve docstr * flat is better than nested * add first test * fix lists and concat->concatenate * add more tests * one more test * add event_repeated param to epochs class * do not put np.nan into int array * better copy handling * also consider 'prior-to-event' codes in selecting new code * test fallback for prior event codes and mixing up * update event_id after removing vals from events * test keeping homogeneous 'prior-to-event' code * revert to 'drop' behavior if duplicates have equal event code * add short example to notes section * make the test more ambitious + reveal issues * only delete indices after loop * fix aud/vis to be same code as vis/aud * fix alex's tests * _handle_duplicate_events -> _handle_event_repeated * use _check_option instead of custom code * remove duplicated tests * simplify pruning of event_id * improve logging msg and readability * cleanup section for setting new events[:, 1] val * event_code -> event_val to disambiguate between key/value/code * refactor: pull _merge_events into its own func * incorporate several points by @jona-sassenhagen * more feedback by jona * improve doc * minor docs * add whatsnew * get rid of superfluous loop * fix pydocstyle * np.testing can go away Co-Authored-By: Alexandre Gramfort <alexandre.gramfort@m4x.org> * using for...else construct

sappelhoff requested review from larsoner and jasmainak August 21, 2019 15:02

sappelhoff added this to To do in woodshole coding sprint (2019) Aug 21, 2019

sappelhoff moved this from To do to In progress in woodshole coding sprint (2019) Aug 21, 2019

jasmainak reviewed Aug 22, 2019

View reviewed changes