fix(LSLClient): corrected "type" case

[oori]

Resolves error when "type = EEG" (uppercase) part of channel info. Otherwise, one gets error: KeyError: "kind must be one of ['eeg', 'mag', 'grad', 'ref_meg', 'misc', 'stim', 'eog', 'ecg', 'emg', 'seeg', 'bio', 'ecog', 'hbo', 'hbr'], not EEG"

[codecov[bot]]

Codecov Report

Merging #17 into master will not change coverage. The diff coverage is 0%.

@@           Coverage Diff           @@
##           master      #17   +/-   ##
=======================================
  Coverage   70.42%   70.42%           
=======================================
  Files          15       15           
  Lines        1606     1606           
  Branches      198      198           
=======================================
  Hits         1131     1131           
  Misses        413      413           
  Partials       62       62

[jasmainak]

possible to add a tiny test? Please also update whats_new.rst

[oori]

I'm not sure how to build the test fixtures for this. This issue happens only when the channel type is given for each of the LSL stream's channels. how would you mock this? lsl_info.desc().child("channels").child("channel") --> .child_value("type")

I noticed lower() is used for the ch_type here: https://github.com/mne-tools/mne-realtime/blob/cb06b789a2560e615dfd5f977590f3be1ab0e729/mne_realtime/lsl_client.py#L104 but two lines later: ch_info.child_value("type") or ch_type and when the LSL stream uses type "EEG" (upper case), we get the KeyError (from here).

I'm not sure if this fix should be here, or upstream on mne's create_info() it expect ch_types to be lower-case (why?). Also, I looked at mne's test here, and there are no "uppercase is invalid" kind of test (ie. 'uppercase' was never specifically considered invalid).

Thanks

[teonbrooks]

@oori good catch. yeah the mismatch is how LSL styles channel types and MNE. I would say that this would be something to add as a note to our documentation for the LSL client so that users would know that the acceptable types are for MNE.

hmm.. it does seem like it should like the capitalization test should be something addressed in mne-python rather than within the mne-realtime only because it pulls from there. If we were to do upper and lower here, we would be breaking the mne-python enforcement.

the proper solution as it stands now would be to make an info with the lower eeg as type. maybe that is what the note should have within the docstring of the LSLClient constructor for info

[jasmainak]

it expect ch_types to be lower-case (why?).

This is historically the case and we're not going to change it now.

Also, I looked at mne's test here, and there are no "uppercase is invalid" kind of test (ie. 'uppercase' was never specifically considered invalid).

You don't need this because MNE throws a pretty explicit error if you give lowercase. Try this:

>>> from mne import create_info
>>> create_info(['EEG001', 'EEG002'], 400., 'EEG') 

[jasmainak]

@teonbrooks feel free to merge if you're happy with this PR!

[teonbrooks]

You don't need this because MNE throws a pretty explicit error if you give lowercase. Try this:

the question is what to do about the uppercase, lowercase passes through, no problem. if this is merged, it won't give that message anymore. essentially we will just coerce to lowercase all the incoming types, which I am ok with.

could you just make a note that within the docstring for under info that the channel types must be one of the approved channel types, cf. create_info

[jasmainak]

yes that sounds like a great idea!

[teonbrooks]

thanks @oori! sorry for the long delay in merging this. had a long period of no dev on this but we are starting actively work on this module again :)

[teonbrooks]

closes #30