Add Annotations to CNT + add code to deprecate stim_channel by massich · Pull Request #6047 · mne-tools/mne-python

massich · 2019-03-12T14:28:09Z

In this PR:

Fix CNT date parsing
Add CNT support in mne.read_annotations
Add stim_channel parameter to mne.io.read_raw_cnt (to later remove stim synthesis)
Fix tests to use stim_channel=False

This needs to go before #6025

mne/io/cnt/_utils.py

mne/io/cnt/cnt.py

mne/io/cnt/tests/test_cnt.py

codecov · 2019-03-12T15:03:33Z

Codecov Report

Merging #6047 into master will increase coverage by 0.04%.
The diff coverage is 83.13%.

@@            Coverage Diff             @@
##           master    #6047      +/-   ##
==========================================
+ Coverage   88.91%   88.95%   +0.04%     
==========================================
  Files         401      402       +1     
  Lines       73013    73123     +110     
  Branches    12135    12151      +16     
==========================================
+ Hits        64922    65050     +128     
+ Misses       5201     5188      -13     
+ Partials     2890     2885       -5

massich · 2019-03-12T17:18:12Z

@wmvanvliet you added the type 3 events in #3911, can you test that events_from_annotations brings you the right onsets?

massich · 2019-03-12T17:59:18Z

for the record, I've checked with files that have cnt and eeglab versions:

32 bits is not working (we can open another PR)
events are ok.
channel locations seem to be ok.

It would be good if we can get cnt files with events of type1 and type3 to be added to the test suit (I only have type2).

…[remove]

massich · 2019-03-12T18:08:47Z

TL;DR

Just for the record, testing with some confidential files + some files on the web running the following

import mne
import pytest
import os.path as op
from datetime import datetime
from mne import __file__ as _mne_file
from mne.tests.test_annotations import _assert_annotations_equal

regular_path = op.join(op.dirname(_mne_file), '..', 'sandbox', 'data')
confidential_path = op.join(regular_path, 'confidential', 'cnt')

flankers_path = op.join(regular_path, '914flankers.cnt')

cnt_files_with_eeglab_pair = [
    op.join(confidential_path, 'BoyoAEpic1_16bit.cnt'),
    op.join(confidential_path, 'cont_67chan_resp_32bit.cnt'),
    op.join(confidential_path, 'SampleCNTFile_16bit.cnt')
]


def test_ensure_meas_date(recwarn):
    raw = mne.io.read_raw_cnt(flankers_path, montage=None,
                              date_format='dd/mm/yy')
    meas_date = (datetime
                 .fromtimestamp(raw.info['meas_date'][0])
                 .strftime('%d/%m/%y %H:%M:%S'))

    assert meas_date == '23/09/07 12:22:15'


@pytest.mark.parametrize('fname', cnt_files_with_eeglab_pair, ids=op.basename)
def test_check_cnt_eeglab_pairs(fname, recwarn):
    raw_eeglab = mne.io.read_raw_eeglab(fname.replace('.cnt', '.set'),
                                        montage=None)
    raw_cnt = mne.io.read_raw_cnt(fname, montage=None)

    assert raw_cnt.info.keys() == raw_eeglab.info.keys()
    xx = object_diff(raw_cnt.info, raw_eeglab.info)
    print(xx)

I get:

The date work properly,
the info difference between cnt and its transformed eeglab version are
- the subject_info anonymization (eeglab is none cnt has empty fields)
- digitization mismatch (but this is beeing addressed somwhere else)
- for one file cnt has a trigger channel marked as bad that does not appear in the eeglab version of the file.

['chs'][63]['cal'] type mismatch (<class 'numpy.ndarray'>, <class 'float'>)
['chs'][63]['coord_frame'] value mismatch (4, 0)
['chs'][63]['loc'] array mismatch
['chs'][63]['unit_mul'] type mismatch (<class 'float'>, <class 'int'>)
['subject_info'] type mismatch (<class 'dict'>, <class 'NoneType'>)

agramfort · 2019-03-12T18:16:07Z

ok it's not a blocker the mismatch with eeglab for my point of view.

…

agramfort · 2019-03-13T07:13:36Z

@massich you have a flake8 error https://travis-ci.org/mne-tools/mne-python/jobs/505338464

agramfort · 2019-03-13T07:16:51Z

mne/io/fieldtrip/tests/helpers.py

-        event_id = list(cfg_local['eventvalue'].astype('int'))
-    else:
-        event_id = [int(cfg_local['eventvalue'])]
+    if 'event_id' not in locals():


calling locals() here seems dangerous makes the code harder to read. Can you see how to avoid it? thx

forget it it's in tests. Not strongly necessary then but just nice to do.

agramfort · 2019-03-13T07:18:57Z

mne/io/cnt/tests/test_cnt.py

+def test_read_annotations():
+    """Test reading for annotations from a .CNT file."""
+    annot = read_annotations(fname)
+    assert len(annot) == 6


I would merge this test with test_compare_events_and_annotations by replacing above the call to _read_annotations_cnt by read_annotations(fname). It will cover the same amount of code and will save time.

wmvanvliet · 2019-03-13T07:44:51Z

@wmvanvliet you added the type 3 events in #3911, can you test that events_from_annotations brings you the right onsets?

@jona-sassenhagen did the actual tests back then. I don't have any CNT files with type 3 events.

…[remove]

larsoner · 2019-03-13T14:17:21Z

@jona-sassenhagen can you test with your event-type-3 CNT file(s)?

larsoner · 2019-03-13T14:20:02Z

mne/io/cnt/_utils.py

+    teeg_parser = Struct('<Bll')
+
+    f.seek(teeg_offset)
+    return Teeg._make(teeg_parser.unpack(f.read(teeg_parser.size)))


Using private _make does not look clean / safe (using private method). Why not just return Teeg(*teeg_parser.unpack(f.read(teeg_parser.size)))

'Cos I did not know how to write it! Thx.

larsoner · 2019-03-13T14:20:17Z

mne/io/cnt/_utils.py

+    def parser(buffer):
+        struct = Struct(struct_pattern)
+        for chunk in struct.iter_unpack(buffer):
+            yield event_maker._make(chunk)


larsoner · 2019-03-13T14:21:44Z

mne/io/cnt/cnt.py

+
+    with open(fname, 'rb') as fid:
+        fid.seek(SETUP_NCHANNELS_OFFSET)
+        (n_channels,) = unpack('<H', fid.read(calcsize('<H')))


typically (and more compactly) we use and prefer np.frombuffer instead of fid.read + unpack

if np.frombuffer took a file descriptor (or the file was lodaded in a buffer) this could be written like this which is really readable:

with open(fname, 'rb') as fid: n_channels = np.frombuffer( fid, dtype=np.uint16, offset=SETUP_NCHANNELS_OFFSET) sfreq = np.frombuffer( fid, dtype=np.uint16, offset=SETUP_RATE_OFFSET) event_table_pos = np.frombuffer( fid, dtype=np.int32, offset=SETUP_EVENTTABLEPOS_OFFSET)

Otherwise np.fromfile but you can not get read of the seek.

with open(fname, 'rb') as fid: fid.seek(SETUP_NCHANNELS_OFFSET) n_channels = np.fromfile(fid, dtype='<u2', count=1)[0] fid.seek(SETUP_RATE_OFFSET) sfreq = np.fromfile(fid, dtype='<u2', count=1)[0] fid.seek(SETUP_EVENTTABLEPOS_OFFSET) event_table_pos = np.fromfile(fid, dtype='<i4', count=1)[0]

But I've no idea which of the 3 is preferable.

frombuffer is what we use in mne/io/tag.py and it works with a file descriptor, we changed it to that from np.fromfile a year or so ago so I think it's the preferred way

(or rather I guess we do frombuffer(fid.read(...), ...) in tag.py)

I would say do whichever works. Having a fid.seek in there is expected/fine

larsoner · 2019-03-13T14:23:40Z

mne/io/cnt/cnt.py

 def read_raw_cnt(input_fname, montage, eog=(), misc=(), ecg=(), emg=(),
                 data_format='auto', date_format='mm/dd/yy', preload=False,
-                 verbose=None):
+                 stim_channel=True, verbose=None):


Here it should be None

Shouldn't default to True to not break people's code? At the beginning I just removed the whole stim. But talking to @agramfort it seems that we cannot get read of the stim in cnt until 0.20, unless we backport the addition of this stim_channel=True to 0.17.3 and then 0.18 to false/none and remove the stim channel.

I would make None an alias for True, where None emits a warning but True does not. But maybe it's not necessary.

larsoner · 2019-03-13T14:26:02Z

mne/io/cnt/cnt.py

+                stim_channel[event_time - 1] = event_id
+
+        else:  # when stim_channel_toggle is False
+            pass


I don't see much value in else: pass because it's implied by just not having it there; and in this case the comment does not add anything because it's just the conditional checked above

larsoner · 2019-03-13T14:27:49Z

mne/io/cnt/cnt.py

+                        stim_channel=stim_channel, n_bytes=n_bytes)
+    else:
+        cnt_info.update(baselines=np.array(baselines), n_samples=n_samples,
+                        n_bytes=n_bytes)


There is redundancy in several conditionals, including here, which could instead be (no need for else at all):

if stim_channel_toggle: ... cnt_info.update(stim_channel=stim_channel) cnt_info.update(baselines=np.array(baselines), n_samples=n_samples, n_bytes=n_bytes)

Seems simpler (makes it clear no matter what, cnt_info is updated with these entries) and is more DRY, no?

larsoner · 2019-03-13T14:30:15Z

mne/io/cnt/cnt.py

+                    _data_stop = start + sample_stop
+                    data_[-1] = stim_ch[_data_start:_data_stop]
+                else:
+                    pass


no need for else: pass

…cked tupple

agramfort · 2019-03-13T20:31:18Z

mne/io/cnt/cnt.py

+    description : array of str, shape (n_annotations,)
+        Array of strings containing description for each annotation. If a
+        string, all the annotations are given the same description. To reject
+        epochs, use description starting with keyword 'bad'. See example above.


Docstring is wrong. You return an annotations

agramfort · 2019-03-13T20:31:56Z

mne/io/cnt/cnt.py

        large amount of memory). If preload is a string, preload is the
        file name of a memory-mapped file which is used to store the data
        on the hard drive (slower, requires less memory).
+    stim_channel : bool (default True)


None not true

massich · 2019-03-13T21:06:08Z

@larsoner merge if you are happy. The code synthesizing the stim channel from the events type 3 is still the same, and there's an upcoming PR fixing the overflow + the 32 bits annotations.

In this PR: - [x] Fix CNT date parsing - [x] Add CNT support in `mne.read_annotations` - [x] Add `stim_channel` parameter to `mne.io.read_raw_cnt` (to later remove stim synthesis) - [x] Fix tests to use `stim_channel=False`

massich · 2019-07-08T15:53:10Z

mne/io/cnt/cnt.py

+                    raise IOError('Unexpected event size.')
+
+                # XXX long NumEvents is available, why are not we using it?
+                n_events = event_size // event_bytes


I guess that #6535 is why.

agramfort reviewed Mar 12, 2019

View reviewed changes

mne/io/cnt/_utils.py Outdated Show resolved Hide resolved

mne/io/cnt/cnt.py Outdated Show resolved Hide resolved

mne/io/cnt/cnt.py Outdated Show resolved Hide resolved

mne/io/cnt/tests/test_cnt.py Outdated Show resolved Hide resolved

Joan Massich added 21 commits March 12, 2019 16:47

Meas date was never parsed, but is still broken

cd16c6e

fix date parsing

e9f8c15

iter

852b177

iter

1296537

NumEvents is funky

901ea0b

Make sure to use the correct Endian + replicate event synthesis

7c9d0bf

TST: only compare events with annotaiton onsets

98eb210

Move reading to the events into cnt.py

bf13cca

ENH: add cnt files to mne.read_annotations

0a78e17

Move the event readers/parser into cnt._utils

1d90107

MAINT: some minor refactoring

6478133

FIX: meas_date has to be a timestamp not datetime

dd436d1

ENH: make the cnt reader closer to eeglab

3b41fd8

ENH: deprecate stim channel synthesis in read_raw_cnt

4a472e5

TST: ensure annotations not synthesized stim

db0b699

FIX: Add annotations to RawCNT

d344883

ENH: update the stim_channel deprecation helper

1ee0c72

ENH: Add stim_channel parameter (defaults to True)

d00eb54

DOC: add test docstrings

bc6a372

FIX: use stim_channel=False for testing fieldtrip

49b66df

Update whatsnew

40fa0e3

massich force-pushed the deprecate_cnt_stim_channel branch from cd14d74 to 40fa0e3 Compare March 12, 2019 15:54

minor changes

5bf4c84

massich marked this pull request as ready for review March 12, 2019 16:04

Joan Massich added 2 commits March 12, 2019 17:24

fix whatsnew

cac9543

Add type3 events

add7a20

massich pushed a commit to massich/mne-python that referenced this pull request Mar 12, 2019

[skip ci][skip travis][skip appveyor][skip azp] Squash mne-tools#6047 …

56dc942

…[remove]

agramfort reviewed Mar 13, 2019

View reviewed changes

minor changes

05a520b

massich pushed a commit to massich/mne-python that referenced this pull request Mar 13, 2019

[skip ci][skip travis][skip appveyor][skip azp] Squash mne-tools#6047 …

2999911

…[remove]

This was referenced Mar 13, 2019

FIX: Montage Index error when importing CNT files #6025

Merged

CNT epoch timestamp access #4715

Closed

massich requested review from jaeilepp and larsoner March 13, 2019 13:40

larsoner reviewed Mar 13, 2019

View reviewed changes

Joan Massich added 4 commits March 13, 2019 16:10

FIX: do not use namedtuple private methods, call constructor with upa…

b883ced

…cked tupple

FIX: DRY!!

f23fea0

FIX: Add stim_channel=None

9287ba5

ENH: remove unpack in favor of frombuffer

a81f5ef

agramfort reviewed Mar 13, 2019

View reviewed changes

Fix docstrings

87e30d6

agramfort approved these changes Mar 13, 2019

View reviewed changes

massich merged commit 9d71dc0 into mne-tools:master Mar 14, 2019

massich deleted the deprecate_cnt_stim_channel branch March 18, 2019 07:38

massich mentioned this pull request Jul 8, 2019

Error reading big CNT file #6535

Closed

massich commented Jul 8, 2019

View reviewed changes

Uh oh!

Conversation

massich commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

massich commented Mar 12, 2019

Uh oh!

massich commented Mar 12, 2019

Uh oh!

massich commented Mar 12, 2019

TL;DR

Uh oh!

agramfort commented Mar 12, 2019 via email

Uh oh!

agramfort commented Mar 13, 2019

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!

wmvanvliet commented Mar 13, 2019

Uh oh!

larsoner commented Mar 13, 2019

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!

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!

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 Mar 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

massich commented Mar 12, 2019 •

edited

Loading

codecov bot commented Mar 12, 2019 •

edited

Loading