EEG-ExPy icon indicating copy to clipboard operation
EEG-ExPy copied to clipboard

refactor: upstreaming improved device classes from erb-thesis

Open ErikBjare opened this issue 3 years ago • 3 comments

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): https://github.com/NeuroTechX/eeg-notebooks/compare/ff5f018a9c9bb2c025a8c41d332ca20d371a7c59...master
    • This was very helpful: https://stackoverflow.com/a/44956845/965332
    • [ ] It's mostly changes in the eeg.py file that need attention (and whatever other files conflict)
      • Like the new _muse_get_recent method, and similar (probably best if whoever wrote them resolve the conflict, I feel very unsure...)
    • [ ] Fix CI issues following merge with develop
  • [ ] Test that things work
    • [ ] For MuseDevice
    • [ ] For BrainflowDevice
  • [x] 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: https://github.com/ErikBjare/thesis/commit/fdaa9491ba522a076f16425772116430646706ac
    • First change in eegnb/devices since I vendored the changes: https://github.com/NeuroTechX/eeg-notebooks/commit/b45201cb4937f4ab3c0c37667c955fd07a375e52
      • Full diff between that commit and now: https://github.com/NeuroTechX/eeg-notebooks/compare/b45201cb4937f4ab3c0c37667c955fd07a375e52...master
      • Changes seem to mostly relate to the FreeEEG32
  • [ ] Figure out how util.py is to be used in the new structure

ErikBjare avatar Apr 13 '21 12:04 ErikBjare

@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 https://github.com/NeuroTechX/eeg-notebooks/pull/88)

ErikBjare avatar Apr 18 '21 09:04 ErikBjare

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

ErikBjare avatar May 05 '22 15:05 ErikBjare

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

ErikBjare avatar May 05 '22 15:05 ErikBjare