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 testing module #324

Closed timonmerk closed 3 weeks ago

timonmerk commented 1 month ago

Many thanks for your first contribution @SamedVossberg!

Currently branch #323 was merged into main. Due to many changes in that branch, some points should be still adressed for the lsl stream generation and testing.

In general I would want to refactor the nm_generator.py module s.t. it's split into three different modules: 1. MNE-LSL Player, 2. MNE-LSL Stream and 3. Offline generator.

Tests:

Adapt then the example notebook. Additional, but for now optional, add plain Open-BCI ipynb example.

I will implement some of those changes in a new branch and notify you about remaining points.

Other points:

toni-neurosc commented 1 month ago

Totally agree on the nm_generator split, the classes inside are very different among them. I would also want to make a diagram of the current Stream class hierarchy because there is some complexity creeping up that we need to keep in check.

For example, the hierarchy NMStream -> _GenericStream -> Stream is really weird right now. _GenericStream docstring says This class can be inhereted for different types of offline streams but really it's handling everything since it contains logic to process LSL streams and also inherits form NMStream which also contains online stream logic.

I also think that raw_data_generator should be just a method in OfflineStream, which does not exist currently but I think it should.

Additionally, _GenericStream lacks a constructor since all the initialization is already handled in NMStream, which I think it proves NMStream holds too much responsability for a supposedly abstract base class.

Finally, I think we should distinguish between LSL streams (which are receiving over-the-network, both the actual LSLstream and the mock stream) and the NMStream, which is really more of a data handler and processor... it's doing a variety of things, except maybe stream data. Things it does:

So, this is A LOT. I think we should figure out a way to define clear boundaries between these tasks, and either redefine our inheritance hierarchy or separate into components that we can combine at runtime depending on settings. Or perhaps both.

SamedVossberg commented 1 month ago

Yes I think that would make totally sense @toni-neurosc and @timonmerk.

As I am still trying to get a complete overview it's quite hard for me to do the refactoring and finding a perfect solution. Unwinding the streams and hierarchies would make absolute sense though @toni-neurosc and would make the code base easier readable and understandable.

@timonmerk I just pushed updated testing which checks for stream sfreq, channel names and types matching the ones specified. I also changed the plot_7_lsl_example.ipynb file to a .py in the format of the other examples.

I will continue with the Tests:

And I will address the point:

timonmerk commented 3 weeks ago

Can we close this @SamedVossberg?

SamedVossberg commented 3 weeks ago

Yes think this is done and can be closed after the changes in #326