Repository navigation
[MRG] Add support for event_type=3 when reading CNT data - #3911
Conversation
|
Closes #3882 :D |
4f11e8c to
69fff4c
Compare
|
Problem is: I don't have an easy means of generating a small test CNT file with these type of events. |
| event_time = (offset - 900 - 75 * n_channels) // (n_channels * | ||
| n_bytes) | ||
| event_time = offset - 900 - 75 * n_channels | ||
| if event_type == 1 or event_type == 2: |
There was a problem hiding this comment.
if event_type in {1, 2} ?
| n_bytes) | ||
| event_time = offset - 900 - 75 * n_channels | ||
| if event_type == 1 or event_type == 2: | ||
| event_time = event_time // float(n_channels * n_bytes) |
There was a problem hiding this comment.
event_time /= float(n_channels * n_bytes)
There was a problem hiding this comment.
it needs to be integer division... does //= exist?
Me neither. But the code is exactly what I did for my own file. |
actually, could you double verify that for me? Do you have a test file yourself for which you know the proper event indices? I'm currently checking this against eeglab but I might get it wrong. |
|
(seems to check out with eeglab on my data) |
wmvanvliet
changed the title
Current coverage is 85.26% (diff: 33.33%)@@ master #3911 diff @@
==========================================
Files 347 349 +2
Lines 61934 61976 +42
Methods 0 0
Messages 0 0
Branches 9494 9501 +7
==========================================
- Hits 53016 52846 -170
- Misses 6255 6447 +192
- Partials 2663 2683 +20
|
wmvanvliet
changed the title
|
@jona-sassenhagen feel free to test this version with any CNT file you may have laying around. It should give exactly the same output as eeglab. |
| if event_type == 3: | ||
| offset *= n_bytes * n_channels | ||
| event_time = offset - 900 - 75 * n_channels | ||
| event_time = int(round(event_time / float(n_channels * n_bytes))) |
There was a problem hiding this comment.
Why floating point operations? These should align well with integers.
There was a problem hiding this comment.
except that they don't. Needed to do this in order to replicate eeglab's output.
There was a problem hiding this comment.
or could eeglab's output be wrong in this case?
There was a problem hiding this comment.
yep, matlab is wrong. It should line up perfectly.
|
Tried it on my own files, works perfectly. |
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
Otherwise LGTM. Please update whats_new.rst
| elif event_type == 2: | ||
| event_bytes = 19 | ||
| elif event_type == 3: | ||
| event_bytes = 19 |
There was a problem hiding this comment.
elif event_type in (2, 3): or do you see the current version as conceptually cleaner?
| if event_type == 3: | ||
| offset *= n_bytes * n_channels | ||
| event_time = offset - 900 - 75 * n_channels | ||
| event_time = event_time // (n_channels * n_bytes) |
.cnt files used by NeuroScan systems support different ways of encoding events (perhaps these reflect old and new versions of the file format). MNE currently only supports types 1 and 2. This PR adds supports for the newer type 3 events.
ebcb016 to
32bfc04
Compare
wmvanvliet
changed the title
|
I think this is ready to merge. I don't believe the travis failures are related to this PR. |
|
LGTM |
|
+1 for merge when Travis is happy |
|
cov plot is breaking things again. I'll try to look soon. In the meantime the CIs are happy otherwise. |
|
Thanks @wmvanvliet |
|
Thanks @wmvanvliet ! |
|
I'm looking at this thread, does anyone have testing data with event_type 2 and 3 ? cross reference #5493 |
7 participants
.cnt files used by NeuroScan systems support different ways of encoding events (perhaps these reflect old and new versions of the file format). MNE currently only supports types 1 and 2. This PR adds supports for the newer type 3 events.
Closes #3882