change find_events to accomodate neuromag bug #1102

dgwakeman · 2014-01-28T21:57:04Z

I have changed the defaults for find_events. This sets the default min_duration to 2 samples. This is useful, because a common Neuromag system "bug" exists which can result in the first sample of an event having a value less than the true trigger value (due to the combined binary trigger numbers). e.g. a trigger of 17 can have one sample of 16 prior to the real trigger value of 17.

dengemann · 2014-01-28T22:11:26Z

@dgwakeman I fear this will change the default behavior for all other systems ... Let's see what Travis says.

larsoner · 2014-01-28T22:18:11Z

Yep, this is an API change so we'll have to deprecate and notify users who have 0 as the default. In the meantime, we could add a config var "MNE_EVENTS_DURATION" to allow users to set a default permanently if people are so inclined.

dengemann · 2014-01-28T22:30:56Z

we could add a config var "MNE_EVENTS_DURATION"

Yea, I think that's what we should do at least. I'm still not sure I understand all consequences of the proposed change, practically ...
Couldn't we have some auto magic here that tries to detect the Neuromag system and the presence of this problem?

dgwakeman · 2014-01-28T22:31:05Z

Well, in order to make the config var useful the units of the variable would need to change (it would need to be in samples not seconds). Personally, I think that makes more sense given the nature of the bug in Neuromag systems (I haven't seen one yet which lasted multiple samples, though please speak up if I am wrong, as I haven't looked at extremely high sampling rates). That though would have a much more drastic impact on peoples work (I suspect). Whereas, I doubt that in practice anyone has a trigger which lasts for a single sample (this change would have a smaller if at all noticeable impact). I think in some cases it may correct for previous errors when people have not noticed them, but that would be it.

mluessi · 2014-01-28T23:09:16Z

I'm -1 on the config file idea. Adding things like this to the config file is "dangerous", i.e., if I work together with someone on the same data we may get different results even though the data, scripts, MNE-Python version are 100% identical.

I propose we comment-out the changes @dgwakeman made and instead issue a warning if min_duration==0 saying it will be changed to None in 0.9.

christianbrodbeck · 2014-01-29T00:54:40Z

The internal _find_stim_steps()' merge parameter is specified in samples, so if you go with min_duration=None and a system-specific solution it might be more logical to build that default into _find_stim_steps().

mluessi · 2014-01-29T03:10:36Z

Thinking about it more, I don't like the idea of adding a warning to 0.8; it would be annoying to see all these warnings which in most cases are not even necessary bc only some systems are affected. How about we add a check to the end of find_events to see if there are additional events that are caused by this bug and if so we display a warning suggesting to use min_duration > 0?

dengemann · 2014-01-29T12:44:52Z

How about we add a check to the end of find_events to see if there are additional events that are caused by this bug and if so we display a warning suggesting to use min_duration > 0?

yes, +1

agramfort · 2014-01-29T12:56:33Z

I agree with the post check and warning in case it happens. The warning should be explicit that min_duration should be > 0

… into find_event_defaults

mluessi · 2014-01-29T21:13:29Z

mne/event.py

@@ -437,7 +437,7 @@ def _find_events(data, first_samp, verbose=None, output='onset',

 @verbose
 def find_events(raw, stim_channel=None, verbose=None, output='onset',
-                consecutive='increasing', min_duration=0):
+                consecutive='increasing', min_duration=None, one_sample=True):


default should be min_duration=0

I would also use a more descriptive name for the new argument, maybe one_sample_warning=True

Or making it more flexible without more code, short_event_warning : int as n_samples below which a warning should be issued

dgwakeman · 2014-01-30T18:22:51Z

Can someone (maybe @dengemann) help me track down these travis errors? I am not familiar with travis (also, I noticed I need to rewrite the error message). Is the test data simulated? If that is the case, it is likely strange test data. Thanks

dengemann · 2014-01-30T18:33:45Z

@dgwakeman it seems your contribution works and detects the bugs you're looking for in arbitrary tests. I thin we should not raise a ValueError here but just issue that exact message using
warnings.warn(msg)

dgwakeman · 2014-01-30T19:45:55Z

@dengemann thanks for your pointers they will be included in the next commit. I have to say, I disagree about just issuing a warning and I feel extremely strongly about this. This is a common error on the most prolific MEG system. One of the errors created in travis by this points out an example in the test_raw.fif file. at sample 30033 to 30035 the system goes from a trigger value of 3 to 2 to 0. This is an example of this problem in the file (this is not a new trigger 2, but a residual value on the trigger channel due to the binary system having the first value set to 0 before the second value). And I have many examples in neuromag data from several sites (also with the problem on the "rising edge"). If users don't check their warnings, they will be averaging their conditions wrong.

dengemann · 2014-01-30T20:08:47Z

This is a common error on the most prolific MEG system. One of the errors created in travis by this points out an example in the test_raw.fif file. at sample 30033 to 30035 the system goes from a trigger value of 3 to 2 to 0. This is an example of this problem in the file (this is not a new trigger 2, but a residual value on the trigger channel due to the binary system having the first value set to 0 before the second value).

@dgwakeman I'm actually not feeling as strongly as you about ValueError VS warn.warnings. The failing tests certainly support your case ;-) and I regularly argue for exceptions rather than for warnings. I think @agramfort suggested a warning here, I might be mistaken though.
If we decide to keep the exception I would raise a RuntimeError instead. To fix the tests you need to modify the failing ones such that the default is overridden. IRRC it was only one failing tests related to events.

dengemann · 2014-01-30T20:12:14Z

@dgwakeman you can search for 'ERROR' in the travis output to see what's going on.

It seems to be easy to fix, it's only one test int mne/fiff/test_event.py

======================================================================
ERROR: Test find events in raw file
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/anaconda/envs/testenv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/anaconda/envs/testenv/lib/python2.7/site-packages/mne-0.8.git-py2.7.egg/mne/tests/test_event.py", line 160, in test_find_events
    assert_array_equal(find_events(raw, output='step', consecutive=True),
  File "<string>", line 2, in find_events
  File "/home/travis/anaconda/envs/testenv/lib/python2.7/site-packages/mne-0.8.git-py2.7.egg/mne/utils.py", line 392, in verbose
    ret = function(*args, **kwargs)
  File "/home/travis/anaconda/envs/testenv/lib/python2.7/site-packages/mne-0.8.git-py2.7.egg/mne/event.py", line 563, in find_events
    " length.") % (n_single))
ValueError: You have 2 events shorter than the short_event_warn. These are very unusual and you may want to set min_duration to a larger value e.g. x / raw.info['sfreq']. Where x > shortest event  length.

agramfort · 2014-01-31T13:20:03Z

mne/event.py

@@ -437,7 +437,8 @@ def _find_events(data, first_samp, verbose=None, output='onset',

 @verbose
 def find_events(raw, stim_channel=None, verbose=None, output='onset',
-                consecutive='increasing', min_duration=0):
+                consecutive='increasing', min_duration=0, 
+                short_event_warn=1):


I would name it

shortest_event

warn suggests a warning and I am happy with the value error.

dgwakeman · 2014-02-03T22:47:46Z

ok, this passed travis (I added a test to confirm this aspect is working). I also added in Alex's comments from previous commit. The only deviation was the comments. I tried to further simplify them (with a minor change to the code to make it easier to explain)

dengemann · 2014-02-03T22:55:19Z

mne/event.py

+    n_short_events = np.sum(np.diff(events[:, 0]) < shortest_event)
+    if n_short_events > 0:
+        raise ValueError("You have %i events shorter than the "
+                          "short_event_warn. These are very unusual and you "


@dgwakeman this may seem nitpicky ... but if we don't care about minor things like this will sum up.
so, one whitespace over-indented here.

dengemann · 2014-02-03T22:56:01Z

besides my nitpick LGTM!

change find_events to accomodate neuromag bug

agramfort · 2014-02-04T07:55:44Z

fixing the nitpick in master myself

thanks @dgwakeman !

dgwakeman · 2014-02-04T18:08:38Z

Hi, I just wanted to highlight that @mluessi was hugely helpful in person here with all of my pesky questions, and credit for pointing this problem out to me was @emanuele. Thanks! D

dengemann · 2014-02-04T18:11:51Z

@dgwakeman I hope we did not discourage you with our nitpicking ;-) looking forward to seeing future PRs

change find_events to accomodate neuromag bug

2f8e018

dgwakeman and others added 3 commits January 29, 2014 11:12

change find_events to accomodate issue an error for one sample events

e8251b8

Merge branch 'find_event_defaults' of github.com:dgwakeman/mne-python…

78752cf

… into find_event_defaults

Incorporated comments from github

7aa0685

mluessi reviewed Jan 29, 2014
View reviewed changes

incorporate comments part deux

289325e

agramfort reviewed Jan 31, 2014
View reviewed changes

more comment incorp and hopefully added tests

654efa3

dengemann reviewed Feb 3, 2014
View reviewed changes

agramfort added a commit that referenced this pull request Feb 4, 2014

Merge pull request #1102 from dgwakeman/find_event_defaults

ff27347

change find_events to accomodate neuromag bug

agramfort merged commit ff27347 into mne-tools:master Feb 4, 2014

dgwakeman mentioned this pull request Jul 14, 2015

MRG: Resampling without losing events #2298

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change find_events to accomodate neuromag bug #1102

change find_events to accomodate neuromag bug #1102

dgwakeman commented Jan 28, 2014

dengemann commented Jan 28, 2014

larsoner commented Jan 28, 2014

dengemann commented Jan 28, 2014

dgwakeman commented Jan 28, 2014

mluessi commented Jan 28, 2014

christianbrodbeck commented Jan 29, 2014

mluessi commented Jan 29, 2014

dengemann commented Jan 29, 2014

agramfort commented Jan 29, 2014

mluessi Jan 29, 2014

christianbrodbeck Jan 29, 2014

christianbrodbeck Jan 29, 2014

dgwakeman commented Jan 30, 2014

dengemann commented Jan 30, 2014

dgwakeman commented Jan 30, 2014

dengemann commented Jan 30, 2014

dengemann commented Jan 30, 2014

agramfort Jan 31, 2014

dgwakeman commented Feb 3, 2014

dengemann Feb 3, 2014

dengemann commented Feb 3, 2014

agramfort commented Feb 4, 2014

dgwakeman commented Feb 4, 2014

dengemann commented Feb 4, 2014

change find_events to accomodate neuromag bug #1102

change find_events to accomodate neuromag bug #1102

Conversation

dgwakeman commented Jan 28, 2014

dengemann commented Jan 28, 2014

larsoner commented Jan 28, 2014

dengemann commented Jan 28, 2014

dgwakeman commented Jan 28, 2014

mluessi commented Jan 28, 2014

christianbrodbeck commented Jan 29, 2014

mluessi commented Jan 29, 2014

dengemann commented Jan 29, 2014

agramfort commented Jan 29, 2014

mluessi Jan 29, 2014

Choose a reason for hiding this comment

christianbrodbeck Jan 29, 2014

Choose a reason for hiding this comment

christianbrodbeck Jan 29, 2014

Choose a reason for hiding this comment

dgwakeman commented Jan 30, 2014

dengemann commented Jan 30, 2014

dgwakeman commented Jan 30, 2014

dengemann commented Jan 30, 2014

dengemann commented Jan 30, 2014

agramfort Jan 31, 2014

Choose a reason for hiding this comment

dgwakeman commented Feb 3, 2014

dengemann Feb 3, 2014

Choose a reason for hiding this comment

dengemann commented Feb 3, 2014

agramfort commented Feb 4, 2014

dgwakeman commented Feb 4, 2014

dengemann commented Feb 4, 2014