neuromodulation / py_neuromodulation

Real-time analysis of intracranial neurophysiology recordings.
https://neuromodulation.github.io/py_neuromodulation/
MIT License
41 stars 9 forks source link

Refactor lsl stream #326

Closed SamedVossberg closed 1 month ago

SamedVossberg commented 1 month ago
timonmerk commented 1 month ago

@SamedVossberg Sorry to mess currently with your PR. I realized however when working today on the documentation, that there was quite a serious issue with building the documentation (a package was not properly installed due to versioning mismatches). Therefore I had to remove nbsphinx and pandoc dependencies.

I realized that this will affect your PR here, and also the examples contribution you've prepared. So I merged my RMAP documentation changes changes into this branch here, which will prevent having to resolve merge conflicts later.

All of those changes don't affect your code.

Just as a heads up, I will now work on embedding your example ipynb into a html file that can be included into the rst, so it's compatible with the current documentation setup.

timonmerk commented 1 month ago

@SamedVossberg Are you still working on changes in this branch? Otherwise the changes look fine imho and I would merge

SamedVossberg commented 1 month ago

Works for me and can be merged.

timonmerk commented 1 month ago

Mhh, so after the discussion here: https://github.com/mne-tools/mne-lsl/issues/262 I think we could get rid of the issue in the LSL example either by adding an additional time.sleep() statement or creating two processes. What do you think @SamedVossberg? Somehow I am in favour of adding the sleep command.. since it seems a bit simpler. But I also need to say that in that case the max. feature sampling rate could likely be 50-70 Hz because of this weird Python Windows issue: https://stackoverflow.com/a/1133888/5060208

SamedVossberg commented 1 month ago

I tried playing with the sleep solution but didn't get a really good result in the example yet... I will look more into it tomorrow though.

timonmerk commented 1 month ago

I think one solution might be simple to add the sleep command as above... I didn't test it yet. But it would be good to not only keep that in the examples / test files, but directly in the stream that it's broadly applicable.

Btw, it's quite crazy how different different OS can be! On Mac I get with s = time.time(); time.sleep(0.00001); e = time.time(); print(e-s) a lower sleep duration of under 1 ms!

toni-neurosc commented 1 month ago

Hi @timonmerk , sorry for butting in, I read your discussion with the maintainer of MNE-LSL and I although I still have to take a look at the LSL stream examples to get an exact idea of the use we want to make of the LSL player, before recurring to time.sleep(), which you already noticed is very unrealiable, I would first quickly attempt to use Python's async computing functions as described here https://docs.python.org/3/library/concurrent.futures.html First probably trying to launch the Player through a ThreadPoolExecutor, and if it does not work then opening a new interpreter using ProcessPoolExecutor.

Sorry I don't have time now to write a test, maybe time.sleep() can solve the issue for now, depends on the urgency, but on the weekend I should be able to put sth together.

PS. Sorry, wrote the previous message too quickly, I'm not sure concurrent.futures is the right tool, could also be that asyncio is the right tool here, but i'd need to research a bit more https://docs.python.org/3/library/asyncio.html

timonmerk commented 1 month ago

While fixing an unused import, I also changed in __init__.py the matplotlib backend.

Beforehand it was set to tkg, which resulted in an error on my machine that matplotlib could not import tkinter.. I could solve it changing the backend to qtagg, but now the tests fail under Linux with:

[gw0] linux -- Python 3.10.14 /home/runner/.venv/bin/python3 worker 'gw0' crashed while running 'tests/test_all_examples.py::test_run_through_all_test[example_filename0]'

I am pretty sure this is related to the matplotlib backend. @SamedVossberg @toni-neurosc Do you think we should set OS specific backends?

I think we are with our machines just testing on Mac and Windows, but it should definitely also pass on Linux..

SamedVossberg commented 1 month ago

I don't fully get why it crashes yet. But if understood right we could just keep the qtagg for Mac and Windows and if on linux run the tkinter version? Or would this result in the tkinter import error thrown before on Linux @timonmerk ?

timonmerk commented 1 month ago

Way too big PR, but many thanks @SamedVossberg and @toni-neurosc! It's really great to have LSL implemented now :tada:

mscheltienne commented 1 month ago

@toni-neurosc Hello, by the way, the classes in mne_lsl already use threading under the hood. As of 1.3, through threading module (with spawning/destruction overhead) and as of 1.4 (I'll release today or next week) with ThreadPoolExecutor which sllightly improves performance and improves a lot stability on CIs.