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

[EEG Browser] Addition of new changes to signal plot of electrophysiology_browser module from EEGNet #8415

Conversation

jeffersoncasimir
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir commented Feb 24, 2023

This PR brings in the latest changes to the signal plot of the electrophysiology_browser module from the EEGNet project.

High-level list of changes

  • Default display is changed to the first 5 seconds of every signal (not full recording) and 16 channels (up from 6)
  • Keyboard shortcuts (legend coming soon)
  • Channel hovering - clearer highlighting for a single signal
  • Dynamic # of displayed channels - up to 64 channels displayed
  • Fixed right-to-left interval selection
  • Added text fields for time interval and interval precision increased
  • List of cursor values toggle feature
  • Zooming controls - including "Zoom to selected region" with 1 click/keypress
  • Stacking toggle feature : to show variance across all channels on a single track
  • ‘Isolate’ toggle feature (highlights a single channel on-hover, when stacked)
  • DC offset toggle feature : filter
  • Overflow toggle feature: signals that wander outside the plot can be made visible
  • Event panel improvements and behaviour consistency

File Changes

Here is a list of the files that were changed, and below them, a list of the major changes to that file. Feel free to ask for more details and I would not mind spending some time walking through some of the motivations behind the changes with the reviewer:

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/color/index.tsx

  • Using different color palettes

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/eeglab/EEGLabSeriesProvider.tsx

  • Changed some default values

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/components/AnnotationForm.tsx

  • Time intervals for Annotation/Events are no longer rounded to the nearest integer

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/components/Epoch.tsx

  • Minimum epoch width is not longer static. It is passed to the component and it scales with the time interval

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/components/EventManager.tsx

  • Gave event visibility a consistent behaviour
  • Added number of visible events
  • Moved ‘too many events to display’ indicator here
  • Height scales with viewer height

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/components/IntervalSelect.tsx

  • Increased interval precision from rounding to the nearest integer
  • Added text fields for changing the interval
  • Refactored logic for changing intervals

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/components/LineChunk.tsx

  • Removed clip path
  • Changed memoization criteria (could likely be improved)
  • Added DC Offset

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/components/ResponsiveViewer.tsx

  • Added overflow toggle

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/components/SeriesCursor.tsx

  • Added hovered channel and event information to CursorContent
  • Removed unused Marker component

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/components/SeriesRenderer.tsx

  • Added zooming features + buttons
  • Added stacked view feature + toggle button
  • Added DC offset feature + toggle button
  • Added overflow visibility toggling feature + button
  • Added key presses for features (legend coming soon)
  • Added changing of channel appearance when hovered
  • Changed channel scaling, with heavy inspiration from EEGLab
  • Added ability to change number of visible channels via dropdown menu

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/logic/dragBounds.tsx

  • Fixed bug where dragging from right to left didn’t work

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/logic/fetchChunks.tsx

  • Refactored variable names
  • Changed interval rounding for chunk fetching

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/logic/filterEpochs.tsx

  • Added helper function for getting epochs (less than ideal design)

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/logic/pagination.tsx

  • Forcing number of channels on page to be number of channels selected

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/logic/timeSelection.tsx

  • Making selection interval round to 3 decimal places with helper function

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/state/bounds.tsx

  • Changed default viewer height and amplitude scale
  • Making interval always [lowerValue, higherValue]

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/state/cursor.tsx

  • Added state for hovered channels
  • Changed cursor payload

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/state/dataset.tsx

  • Changed reference name for initial channel limit

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/index.tsx

  • Added new epic for hovered channels

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/types.tsx

  • Added new type

modules/electrophysiology_browser/jsx/react-series-data-viewer/src/vector/index.tsx

  • Added and modified some constants and initial values

@laemtl laemtl removed the State: Needs work PR awaiting additional work by the author to proceed label Mar 22, 2023
Copy link
Contributor

@laemtl laemtl left a comment

Choose a reason for hiding this comment

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

Thanks @jeffersoncasimir for this amazing work. A few comments:

  • Can those warnings be fixed?

npm run compile:

/var/www/loris/modules/electrophysiology_browser/jsx/react-series-data-viewer/src/eeglab/EEGLabSeriesProvider.tsx
  21:9  warning  'updateFilteredEpochs' is defined but never used  @typescript-eslint/no-unused-vars

/var/www/loris/modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/logic/dragBounds.tsx
  31:38  warning  'fromState' is defined but never used  @typescript-eslint/no-unused-vars
  50:1   warning  Missing JSDoc @param "root0" type      jsdoc/require-param-type
  51:1   warning  Missing JSDoc @param "root0."0"" type  jsdoc/require-param-type
  52:1   warning  Missing JSDoc @param "root0."1"" type  jsdoc/require-param-type
  55:43  warning  '_' is defined but never used          @typescript-eslint/no-unused-vars

/var/www/loris/modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/store/logic/filterEpochs.tsx
  121:1  warning  The type 'Epoch' is undefined  jsdoc/no-undefined-types
  125:1  warning  The type 'Epoch' is undefined  jsdoc/no-undefined-types

console

VM225 react_devtools_backend.js:2655 Warning: Each child in a list should have a unique "key" prop.
Check the render method of `SeriesRenderer`. See https://reactjs.org/link/warning-keys for more information.

VM225 react_devtools_backend.js:2655 Warning: Internal React error: Expected static flag was missing. Please notify the React team.
    at SeriesRenderer (https://10.31.82.74/electrophysiology_browser/js/electrophysiologySessionView.js:43454:25)

VM225 react_devtools_backend.js:2655 Warning: Internal React error: Expected static flag was missing. Please notify the React team.
    at SeriesCursor (https://10.31.82.74/electrophysiology_browser/js/electrophysiologySessionView.js:43075:22)

VM225 react_devtools_backend.js:2655 Warning: Each child in a list should have a unique "key" prop.
Check the render method of `TimeMarker`. See https://reactjs.org/link/warning-keys for more information.
  • Any reason why the upper axis line is gone?

Screenshot 2023-03-23 at 12 40 43 PM

  • If the older images are unused, can you delete them?

  • I wasn't able to isolate a channel. Let me know if this works on your side, I may not trigger the feature appropriately.

@laemtl laemtl assigned jeffersoncasimir and unassigned laemtl Mar 23, 2023
@jeffersoncasimir jeffersoncasimir force-pushed the 2023_02_24_electrophysiology_browser_module_eegnet branch from d225595 to 223fc33 Compare March 28, 2023 15:03
@jeffersoncasimir
Copy link
Contributor Author

jeffersoncasimir commented Mar 28, 2023

I addressed all the warnings and deleted the unused images.

  • Any reason why the upper axis line is gone?

The top axis was actually not originally there (see older demo) but I had added it back before I implemented overflow toggling. I did not find an elegant way to bring it back in while having consistent appearance between the two modes so I left it looking as it was.

  • I wasn't able to isolate a channel. Let me know if this works on your side, I may not trigger the feature appropriately.

How the 'isolate' feature works:

  1. Press 'Stack' to stack the channels
  2. Press 'Isolate' to enable 'isolate' mode
  3. Hover over a channel name on the left hand side

Let me know if there is still an issue after following these steps

@laemtl laemtl force-pushed the 2023_02_24_electrophysiology_browser_module_eegnet branch from e90140d to e014119 Compare March 29, 2023 16:43
jeffersoncasimir and others added 5 commits March 29, 2023 12:47
PR aces#8242 modified the electrodes DB schemas and since then, the EEG Browser is broken.
This fixes the issue by rewriting the query logic.
Also fixes a few issues with the download buttons and the layout.
@laemtl laemtl added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Apr 4, 2023
@laemtl laemtl changed the base branch from main to 25.0-release April 4, 2023 15:12
@laemtl laemtl removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Apr 4, 2023

### Stacked View
Hovering channel names while in 'stacked' or 'spread (default)' view will thicken the respective signal(s). While in stacked view, a feature called "Isolate" becomes available. [[Isolate Mode Demo](#isolate-mode)]
<br/><br/>![Stacked View](./../../images/signal-stacked.png) <br/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think these images should be here. The sentences make sense without them and I don't think they add enough value to justify the cost of adding binary files to our github repo.


### Isolate Mode
Hovering channel names while in 'isolate' mode will make that signal the only visible signal on the plot.
<br/><br/>![Isolate Mode](./../../images/signal-isolated.png) <br/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think these images should be here. The sentences make sense without them and I don't think they add enough value to justify the cost of adding binary files to our github repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @driusan - I have an updated commit ready to push, but just for context, these images were added to be consistent with @laemtl's documentation, and I think, requested originally by Leigh.
@laemtl should we move these elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if we have the option to host the files on a server to avoid committing the images. Documentation wise, I think those images were a good addition to document the module's new features.

Copy link
Contributor

Choose a reason for hiding this comment

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

@driusan This PR updates a README that was already containing images. Maybe we can discuss the images issue later to not hold this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, once images are committed they're in the repo forever and need to be downloaded with every clone.

Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

I skimmed most of the code and other than the binary files being committed to GitHub I think this can probably go in.

@laemtl laemtl requested a review from driusan April 6, 2023 16:45
@ridz1208 ridz1208 mentioned this pull request Apr 11, 2023
14 tasks
@xlecours xlecours added the Critical to release PR or issue is key for the release to which it has been assigned label Apr 11, 2023
@laemtl laemtl assigned driusan and unassigned jeffersoncasimir Apr 18, 2023
@laemtl
Copy link
Contributor

laemtl commented Apr 18, 2023

@driusan Images have been migrated on a remote server, the PR is good to go.

@laemtl laemtl force-pushed the 2023_02_24_electrophysiology_browser_module_eegnet branch from 7654766 to d2e140a Compare April 24, 2023 13:12
@driusan driusan merged commit 2fc4afe into aces:25.0-release Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical to release PR or issue is key for the release to which it has been assigned Release: Add to release notes PR whose changes should be highlighted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants