-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: upstreaming improved device classes from erb-thesis #93
base: develop
Are you sure you want to change the base?
Conversation
1f145ec
to
feca4c3
Compare
@JadinTredup I got this PR in an almost mergeable state. The only thing left is to patch the FreeEEG32 stuff that was added after I forked If we get the FreeEEG32 stuff fixes soon, I think we might actually be able to merge this soon. I also added more testing (with pytest), which would be really nice to get merged. (oops, apparently I had similar changes in #88) |
Pushed an incomplete merge commit. Needs fixing (and careful review). |
@JohnGriffiths @JadinTredup Looks like delaying this made the conflicts a lot worse... I tried to do my best to clean it up, but some issues left and it's 6PM here, so I'm off duty. Will take another look & continue tomorrow. I would appreciate if you could both have a look and check if the general direction is good, and if the API looks reasonable (or if anything is unclear). |
Ok so better late than never... @ErikBjare I have looked through the code here and like the ideas. Will do some tests soon and keen to merge to `develop' soon if no obvious issues. |
Replaces #92 (switched to branch in the NeuroTechX repo instead of my fork)
Fixes #43
Fixes #77
TODO
eeg.py
file that need attention (and whatever other files conflict)eeg.py
between fork and noweegnb/devices
since I vendored the changes: b45201cutil.py
is to be used in the new structure