Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[EEG Browser] Signal Visualization, Events, Electrode map and EEG split files support #7387

Merged
merged 8 commits into from
Sep 24, 2021

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Mar 11, 2021

This PR adds dynamic visualization of EEG based on a previous project started by Armin Taheri (https://github.com/aces/react-series-data-viewer).
These changes and include cleanup of older code in the Electrophysiology Browser and new features like viewing the list of Events, and an Electrode Map dynamically plotted from the BIDS-compliant coordinates file.

The visualization component uses svg (Visx|D3) and introduces a new stack of dependencies:

Ramda (https://ramdajs.com)

A practical functional library for JavaScript programmers.

Redux (https://redux.js.org)

A Predictable State Container for JS Apps

Visx (https://airbnb.io/visx)

A collection of expressive, low-level visualization primitives for React.

RxJS (https://rxjs-dev.firebaseapp.com/guide/overview)

RxJS is a library for composing asynchronous and event-based programs by using observable sequences.
It provides one core type, the Observable, satellite types (Observer, Schedulers, Subjects) and operators to allow handling asynchronous events as collections.

TypeScript (https://www.typescriptlang.org)

A static type checker for javascript.

Preview

Screenshot_2021-04-09 LORIS Demonstration Database - Electrophysiology Browser - View Session(6)

To test

To discuss

This PR contains the protobuf file used to generate the frontend (js) and backend (python, used in Loris MRI) chunk message encoder/parser. I'm not sure this file should be stored here and what would be a good compilation pipeline.

To fix

The electrode map needs to support a coordinate system to perform appropriate rotations.

@laemtl laemtl marked this pull request as draft March 11, 2021 18:22
@laemtl laemtl marked this pull request as ready for review March 11, 2021 18:22
@laemtl laemtl force-pushed the 2021-03-11-eeg-visualization branch 2 times, most recently from c9c2842 to 3138e92 Compare March 11, 2021 18:31
@christinerogers christinerogers added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 11, 2021
@laemtl laemtl force-pushed the 2021-03-11-eeg-visualization branch 4 times, most recently from 0b73155 to 0630043 Compare March 11, 2021 18:53
@laemtl laemtl added the Feature PR or issue introducing/requiring at least one new feature label Mar 11, 2021
@laemtl laemtl force-pushed the 2021-03-11-eeg-visualization branch 12 times, most recently from 900d811 to 2eccf3a Compare March 12, 2021 08:13
@laemtl laemtl added the Blocked PR awaiting the merge, resolution or rejection of an other Pull Request label Mar 12, 2021
@laemtl laemtl force-pushed the 2021-03-11-eeg-visualization branch 4 times, most recently from afaa3a8 to e3c7a44 Compare March 17, 2021 20:46
@laemtl laemtl removed Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix Blocked PR awaiting the merge, resolution or rejection of an other Pull Request labels Mar 17, 2021
@laemtl laemtl force-pushed the 2021-03-11-eeg-visualization branch from e3c7a44 to becdd49 Compare March 17, 2021 20:53
@laemtl laemtl force-pushed the 2021-03-11-eeg-visualization branch from 23c0f78 to 6eaeaa7 Compare August 4, 2021 21:01
@laemtl
Copy link
Contributor Author

laemtl commented Aug 5, 2021

@cmadjar, @driusan Fixed.

@laemtl laemtl removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Aug 5, 2021
@laemtl laemtl changed the title [EEG Browser] Signal Visualization, Events, Electrode map and code cleanup [EEG Browser] Signal Visualization, Events, Electrode map and EEG split files support Aug 5, 2021
@laemtl
Copy link
Contributor Author

laemtl commented Aug 30, 2021

@maltheism Do you have time to review?

@maltheism
Copy link
Member

@laemtl one quick question I thought of while I'm now testing this PR. You have protobuf support for macOS and ubuntu. I'm just wondering if centos that LORIS supports would also have something for protobuf?

@maltheism
Copy link
Member

@laemtl when visiting http://localhost/electrophysiology_browser/ I get the following in browser console:
Warning: Failed prop type: Invalid prop value supplied to SelectElement.

Warning: Failed prop type: Invalid prop value of type boolean supplied to TextboxElement, expected string.

@laemtl
Copy link
Contributor Author

laemtl commented Aug 30, 2021

@laemtl when visiting http://localhost/electrophysiology_browser/ I get the following in browser console:
Warning: Failed prop type: Invalid prop value supplied to SelectElement.

Warning: Failed prop type: Invalid prop value of type boolean supplied to TextboxElement, expected string.

Good point but unrelated to my changes in this PR. I suggest to open an issue to address those errors in all modules.

@laemtl
Copy link
Contributor Author

laemtl commented Aug 30, 2021

@laemtl one quick question I thought of while I'm now testing this PR. You have protobuf support for macOS and ubuntu. I'm just wondering if centos that LORIS supports would also have something for protobuf?

Yes, for CentOS users can use the third link:

install protoc:

sudo apt-get install -y protobuf-compiler (Ubuntu > 16.10)
brew install protobuf (Mac)
https://github.com/protocolbuffers/protobuf/blob/master/src/README.md

@maltheism
Copy link
Member

maltheism commented Aug 30, 2021

Hi @laemtl, thanks for the answers and you're right I didn't realize you weren't using those components while the warnings are coming from the core filter/table. It's taking me some additional time to setup the MRI pipeline and so far I can only visit /electrophysiology_browser for testing.

Copy link
Member

@maltheism maltheism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving the javascript part of the PR. This PR is large considering all of the work done. Other things will be necessary like handling rare errors but that can be done in separate PRs after this PR is merged.

@cmadjar ready for your manual tests. :)

@laemtl laemtl added the Critical to release PR or issue is key for the release to which it has been assigned label Aug 31, 2021
@christinerogers christinerogers removed the Needs Documentation PR awaiting proper documentation of the changes label Sep 20, 2021
@christinerogers
Copy link
Contributor

@driusan - ready for your review -- this has 2 approvals and is needed for the release. thanks!

@driusan driusan merged commit 57aa485 into aces:main Sep 24, 2021
@ridz1208 ridz1208 added this to the 24.0.0 milestone Nov 4, 2021
@christinerogers christinerogers removed the Critical to release PR or issue is key for the release to which it has been assigned label Nov 9, 2021
driusan pushed a commit that referenced this pull request Dec 1, 2022
This fills in some gaps in the code relating to EEG annotations. It connects the front-end architecture from #7433 to the annotation form that was added in #7387. The result is that a user can now add new annotations from the EEG Browser.

Based on @jesscall's work in PR #7829
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) Document at Release PR adds or changes a feature such that the wiki (or other documentation) must be updated Feature PR or issue introducing/requiring at least one new feature Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants