Merge changes to the device abstraction ("EEG") class from my repo

NeuroTechX / EEG-ExPy

EEG Experiments in Python

https://neurotechx.github.io/EEG-ExPy/

BSD 3-Clause "New" or "Revised" License

437 stars 124 forks source link

Merge changes to the device abstraction ("EEG") class from my repo #43

Open ErikBjare opened 3 years ago

ErikBjare commented 3 years ago

My stuff is in: https://github.com/ErikBjare/thesis/blob/master/src/eegwatch/devices/

Includes a bunch of minor changes, including:

Added type annotations
Support for recording Muse PPG/ACC/GYRO streams

TODO

[ ] start/stop is different for Brainflow vs Muse devices, this should maybe be refactored, but at least be documented.
[ ] I've ran the black formatter on all my code, so merging would be easiest after similar formatting has been applied on the eeg-notebooks project (like in #30)
[ ] Needs a way to enable PPG/ACC/GYRO streams (without hardcoding)

JadinTredup commented 3 years ago

@ErikBjare it appears to me that you have completed this or is there still more work to be done?

ErikBjare commented 3 years ago

@JadinTredup Nope, never got around to making a PR.

I've made some pretty major changes, and working on a few more, almost to the point where I'm not sure if it makes sense to upstream it. But if it's of interest I'll consider it: https://github.com/ErikBjare/thesis/tree/master/src/eegwatch/devices

JohnGriffiths commented 3 years ago

@ErikBjare - what's your thoughts. Do you think these changes should be added or is divergence too great?

ErikBjare commented 3 years ago

@JohnGriffiths I'm not sure, I'd love it if you had a look at it and told me what you think.

Code is here: https://github.com/ErikBjare/thesis/tree/master/src/eegwatch/devices

Basically I created an abstract class EEGDevice that is implemented by the classes MuseDevice and BrainflowDevice. It simplifies a lot of the code imo, no more if statements to check if using one type of device or another, for example.

I also implemented a check method (only for MuseDevice so far) which does a very basic signal check (old method, not the newer one I linked you recently) and returns the list of bad channels, but that probably doesn't belong in the class (at least not in its current state).

Edit: Oh, and it also uses the bleak backend in muse-lsl by default (not yet merged upstream in muse-lsl afaik).

JadinTredup commented 3 years ago

@ErikBjare I am going to look this over today. Thank you so much for doing this. One of my biggest take aways from my internship development-wise has been implementing classes like this and so I was intending to do this at some point.

JadinTredup commented 3 years ago

@JohnGriffiths I like this a lot and think it improves our flexibility/scalability in the future. If you're okay with it I say we set up another branch and work on merging this.

ErikBjare commented 3 years ago

@JadinTredup Branch set up in #92, comments welcome!