-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fixed excluding an event that happens first and once. #11674
Conversation
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
thx @nafraw can you look into the failing unit tests? also you'll need to update the latest.inc file to document the bug fix. thanks |
Hi @agramfort I updated the latest.inc file. I am not sure if I missed something. There are still errors with the build_doc. I just noticed that this bug is basically the same as #11626. I don't know if np.count_nonzero(event) is better, but for sure <= 1 is not going to work for the problem I described. I checked one of the failed test and manually read test_egi.raw using raw=read_raw_egi() as in the unit test with output: Then I use mne.find_events(raw). The output is: It looks fine that events are properly read. Similar to the opinion of #11626, I don't see why the test file should give a warning, ""Did not find any event code with more than one " "event."", The exact issue of this bug is that an event may only happen once and first in some recordings; EGI supports network event, which will be only one sample at the current version (even xml file has a duration tag). The described scenario certainly may happen. A recording with only one event is also technically possible, although rare. As a result, who decides that an event channel has only one event should be excluded must decide what to do or explain if some specific channels with only one event must be excluded. Otherwise, this issue will never end. |
sorry I don't have time to look |
For the documentation build, the issue is that you are not listed in For the tests, this test is not passing: https://github.com/mne-tools/mne-python/blob/52d1e3e016e5caadb040eb3fa30790a8895d8732/mne/io/egi/tests/test_egi.py#L208C1-L209 But it's not completely clear for me what you are fixing and why this test is now failing. Is it because the fix is wrong or because the test is wrong? Could you share a (small) file with a small code snippet which demonstrate:
|
I added my name and pushed. This is an example (not super small) file without meaningful EEG data, but may be deleted after some time. You need to unzip with password: You can use the codes below to load data. The test_egi.raw is the failed test file. With the provided file, it has 5 events as can be read from the Events_[...].xml file. Before fixing, MNE ignored the first singular event, 444, leading to only 4 events. After fixing, it returns 5 events. However, for unknown reason and to my understanding, MNE thinks the first and singular* event is not an event and should throw a warning, "Did not find any event code with more than one " "event.". It seems the test file has a singular event and should throw a warning, but now it cannot as this is what I fixed. How I fixed is clearly described in #11672 *: based on the codes I understand, only happens if an event happens first in the file and only once, since the code is using event.sum() <= 1 to decide. If an event happens only once, but not the first, it is not a problem, as the event code itself is already 2 or larger.
|
I can confirm/reproduce the issue. The check for
I should add that I haven't personally worked with NetStation hardware so I'm not very familiar with the MFF folder format. This means I can't confidently say whether the original check is sensible or not. I can imagine cases where one might want to exclude an event if and only if it is the first one and it occurs only once (e.g. a standard system-generated event that doesn't reflect experiment design) but I can't tell if that is the logic/motivation here. There are a few other open issues about failures to read MFF files: #11380, #11188, #11626 (same issue as here), #11627 (draft PR with similar fix as here). Since the changes here seem to be breaking other tests, I've just merged @mscheltienne suggested here that we migrate our EGIMFF Raw reader to use mffpy under the hood (which nowadays is already a dependency for reading Epoched MFF files). That seems like the more promising (though harder) approach, and it seems like we should make that a priority. @nafraw @jackz314 @ephathaway @jusjusjus @christianbrodbeck do any of you have time/interest in refactoring our Raw EGI reader to use mffpy? |
Indeed moving to using To move the present PR forward, one option would be for someone to use |
I did that and indeed the event is there in mffpy (though they don't read in events by default with their |
Thanks for looking into this! I don't have the capacity to work on this right now but I would be interested in adapting mffpy down the line if I have more time and no one else is working on it. I think your reasoning for the original check (standard system-generated event) makes sense but I don't think it should be the default IMO. In our case, we had events indicating the start of experiments that were omitted due to this check. For a short-term fix, I think we can disable the check by default, and add the option to explicitly exclude the first one-shot event if people still want the old behavior. |
Reference issue
Fixes #11672.
What does this implement/fix?
Reading EGI file may accidentally excludes an event that happens first and once. This fix made "event.sum() <= 1" as "event.sum() < 1" as the mentioned event above has event.sum() == 1
Additional information
More details are explained in #11672 already.