[MRG] RealTime client refactor#6141
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6141 +/- ##
======================================
Coverage 89% 89%
======================================
Files 410 411 +1
Lines 73885 73885
Branches 12256 12249 -7
======================================
Hits 65765 65765
- Misses 5224 5228 +4
+ Partials 2896 2892 -4 |
|
This pull request introduces 10 alerts when merging 13c89af into 35d7c53 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
|
This pull request introduces 10 alerts when merging 8fba9e9 into aa74f02 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
|
This pull request introduces 1 alert when merging 6763f7f into 5610605 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
|
@jasmainak this is marked MRG now, can you take a look? |
110dffe to
a127971
Compare
|
I need to take a closer look by running some of the realtime examples. Could we mark this for the sprint? @teonbrooks you should not tag someone in a commit if you plan to rebase :p I got like 10 emails because you rebased lol |
|
@jasmainak i didn't tag you, lol. and i didn't realize you got emails about the rebasing too, sorry about that |
mne/realtime/mock_lsl_stream.py
Outdated
| """ | ||
|
|
||
| def __init__(self, host, n_channels=8, ch_type="eeg", sfreq=100, | ||
| testing=False): |
There was a problem hiding this comment.
I would instead suggest that it take a raw object as an argument. Then you don't need n_channels, ch_type, sfreq etc.
There was a problem hiding this comment.
agreed, it should take any raw instance (subject to any necessary constraints of the streamer, e.g. only EEG channels or something) and "play it back"
There was a problem hiding this comment.
that is totally possible. looking at the code, i realize that the name mockrtclient is either a misnomer or it is working as both the server and the client. I feel that these two functionality should be separated. what i mean is that the client shouldn't be serving up data, it should be a listener that can pass the data along to other functionalities.
there should be stream generator that takes a raw file and send it to LSL or TCP/IP. The client should then be able to listen using their protocols. it could combine with the existing stim_server_client module for the latter protocol
There was a problem hiding this comment.
I agree it would be cleaner to have a mock server emulate mne_rt_server but this is written in CPP. Writing a mock server to emulate this may not be so straightforward, and certainly was not for someone who had learned python 10 days ago ;-) But this could perhaps be a point of collaboration with @LorenzE since he may be familiar with the MNE-CPP code and the protocol for mne_rt_server.
|
This pull request introduces 2 alerts when merging 3f8f05e into b3dfd7a - view on LGTM.com new alerts:
Comment posted by LGTM.com |
|
@teonbrooks the CIs are not happy I'll test tomorrow if the fieldtrip examples are not broken. Otherwise, LGTM |
|
Also, you'd need to implement this to work with RtEpochs so that it is feature complete. We can open an issue when this PR is merged so that we don't forget |
|
Also, could we triage based on whether stream ID or stream name is provided? If this is not possible, perhaps we should add a bit of documentation how the stream ID can be obtained. cc @akovrig |
currently the test is breaking when it comes to using the RtEpochs object.
Windows runs into a problem with multiprocessing: 'https://stackoverflow.com/questions/50079165/'
this is compatible across platforms
|
LGTM. Whoever wants to feel productive this morning can merge :-) |
|
Thanks @teonbrooks @jasmainak ! |
|
Broke CircleCI: It gets killed shortly thereafter. Maybe there is a memory problem? CircleCI does not have a ton of memory. Can you fix it? Maybe it's not related to memory and is something else? |
* Add an example using the LSLClient n_chan --> n_channels * Refactor FTClient; Add MockLSLStream, refactor test to use mock stream * update reference and whats new * fixing some errors * update style * temp * improvements to the realtime module currently the test is breaking when it comes to using the RtEpochs object. * minor fix * move the RtEpochs testing to separate PR * cleanup * fix the way super is called * updated the MockLSLStream to take raw instance * add time dilation factor, cleanup * add more info on lsl identifier * address ci * skip running test with multiprocessing on windows Windows runs into a problem with multiprocessing: 'https://stackoverflow.com/questions/50079165/' * cleaned up the windows check * update the pylsl requirement to 1.12 this is compatible across platforms
|
Maybe a bit late, just stumbled upon this one: |
|
@stfnrpplngr and others reading this - i just wanted to provide a disclaimer about https://github.com/kaczmarj/rteeg. i don't have time to maintain that package now, and frankly, i wrote that when i just started out programming, so it probably doesn't follow good practices... i've just read through this PR, and i think it makes my rteeg package redundant. thanks @teonbrooks for all your work on this! |
* Add an example using the LSLClient n_chan --> n_channels * Refactor FTClient; Add MockLSLStream, refactor test to use mock stream * update reference and whats new * fixing some errors * update style * temp * improvements to the realtime module currently the test is breaking when it comes to using the RtEpochs object. * minor fix * move the RtEpochs testing to separate PR * cleanup * fix the way super is called * updated the MockLSLStream to take raw instance * add time dilation factor, cleanup * add more info on lsl identifier * address ci * skip running test with multiprocessing on windows Windows runs into a problem with multiprocessing: 'https://stackoverflow.com/questions/50079165/' * cleaned up the windows check * update the pylsl requirement to 1.12 this is compatible across platforms
following up on #6120. It also adds a mock client and an example using LSL. This refactors the FieldTripClient and the lsl test.