Skip to content
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

Draft
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

ErikBjare
Copy link
Collaborator

@ErikBjare ErikBjare commented Apr 13, 2021

Replaces #92 (switched to branch in the NeuroTechX repo instead of my fork)

Fixes #43

Fixes #77

TODO

  • Resolve conflicts caused by changes to master since PR opened (that need to be integrated as part of a merge/rebase): ff5f018...master
  • Test that things work
    • For MuseDevice
    • For BrainflowDevice
  • Ensure that tests and typechecking runs correctly in CI
  • Check differences to eeg.py between fork and now
    • When I vendored the code into my thesis repo: ErikBjare/thesis@fdaa949
    • First change in eegnb/devices since I vendored the changes: b45201c
      • Full diff between that commit and now: b45201c...master
      • Changes seem to mostly relate to the FreeEEG32
  • Figure out how util.py is to be used in the new structure

@ErikBjare ErikBjare changed the title refactor: upstreaming improved device classes from erb-thesis (WIP) refactor: upstreaming improved device classes from erb-thesis Apr 18, 2021
@ErikBjare
Copy link
Collaborator Author

ErikBjare commented Apr 18, 2021

@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 eegnb/devices (see top comment for links to diffs).

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)

@ErikBjare ErikBjare marked this pull request as draft October 5, 2021 14:34
@ErikBjare ErikBjare changed the base branch from master to develop April 21, 2022 15:57
@ErikBjare
Copy link
Collaborator Author

Pushed an incomplete merge commit. Needs fixing (and careful review).

@ErikBjare
Copy link
Collaborator Author

ErikBjare commented May 5, 2022

@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).

@JohnGriffiths
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge changes to the device abstraction ("EEG") class from my repo Add code coverage analysis
2 participants