Skip to content

Conversation

@jeffersoncasimir
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir commented Oct 23, 2025

This PR adds the latest EEG features from EEGNet.

Notably, this includes a major overhaul to the Event Panel and adds the ability to "Endorse" HED tags - as well as caveat and/or comment. Filter/search also added.

Additional features to support event channels: i.e. Events can be created/tagged for subsets of channels.

  • Toggle selection from montage view
  • Toggle selection from signals
  • time segment highlighting is now channel-based (all unless defined)

Downloadable dynamic events.json file (Dataset Tag Manager) updates itself after new tags are added/used.

Screenshots:
Screenshot 2025-10-16 at 1 24 27 PM
Screenshot 2025-10-16 at 1 24 42 PM
Screenshot 2025-10-16 at 1 24 50 PM
Screenshot 2025-10-16 at 1 25 29 PM
Screenshot 2025-10-27 at 11 17 22 AM
Screenshot 2025-10-27 at 11 17 31 AM

@github-actions github-actions bot added Language: SQL PR or issue that update SQL code RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset Language: PHP PR or issue that update PHP code Language: Javascript PR or issue that update Javascript code Module: electrophysiology_browser PR or issue related to electrophysiology_browser module labels Oct 23, 2025
@jeffersoncasimir jeffersoncasimir removed Language: SQL PR or issue that update SQL code RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset labels Oct 23, 2025
@christinerogers christinerogers added Release: Document at release PR whose changes need to be documented in the wiki (or other documentation) at release Priority: High PR or issue should be prioritised over others for review and testing labels Oct 24, 2025
@christinerogers christinerogers added this to the 27.0.0 milestone Oct 24, 2025
@CamilleBeau CamilleBeau removed this from the 27.0.0 milestone Oct 27, 2025
@CamilleBeau CamilleBeau requested a review from regisoc October 27, 2025 19:13
@christinerogers
Copy link
Contributor

@jeffersoncasimir how about adding to the PR description something like : generates updated and downloadable BIDS-compliant events.json based on current tags, via the Dataset Tag Manager submodule

@SantiagoTG SantiagoTG requested review from nicolasbrossard and removed request for regisoc October 31, 2025 14:02
@christinerogers christinerogers added this to the 28.0.0 milestone Nov 7, 2025
Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Note also: I edited the PR description for a bit more clarity esp for the release notes.

@jeffersoncasimir want to let me know for Monday if you need more info to reproduce these?

where I tested it on https://jcasimir-test-27.loris.ca/electrophysiology_browser/sessions/2161?outputType=raw:

  • Dataset Tag manager button not visible - not testable.
  • Tagged by / Endorsed by / Caveat By -> these lists are empty, feature is not usable. Is a patch / something else missing?
  • Adding a second HED Artifact tag doesn't work: Message: fill all the other tags first -- also if it's a warning /error it shouldn't be green.

Did more testing on : https://jcasimir-test-27.loris.ca/electrophysiology_browser/sessions/167?outputType=raw

  • Adding any HED Artifact tag doesn't work
  • Endorsement problem w keystrokes: If I use the key strokes (^ C or M) after clicking in the 3rd HED tag, it changes the 1st HED tag visible. Please disable and remove the hover text regarding keystrokes if it can't be fixed for this release
  • can't undo my own Caveat.
  • tried to remove 2 tags added by Data authors. It showed me the X next to each orange tag (suggesting I could) so I clicked. then clicked Submit. got an error message but no details. The orange tags disappear and the submit button greyed out, but when i navigate elsewhere I am asked to confirm that i will lose changes. If i refresh the orange tags are back (delete failed)
  • from the HEd endorsement panel -- the edit pencil likewise suggests I can edit tags but I don't have that permission as admin. either the messaging or the behaviour needs clarification.
  • unclear how to add a tag only to specific channels, without changing the channel scope of all tags. When I go to add a tag for only a few channels it changes the channels on all the existing tags added by Data Authors which are otherwise uneditable (or so it seems)

@jeffersoncasimir
Copy link
Contributor Author

jeffersoncasimir commented Nov 10, 2025

@christinerogers please retest and amend the list to remove items that were due to the timing in which you were testing (like a crucial button not being visible). You apparently caught my testing VM in a window where I was rebuilding to test another PR. Make sure to use the OTT recordings and not the random other one.

Please note, an instance-level HED tag must be manually added (or present in the recording, which is not the case here) in order to appear on the list of HED endorsements. To test the (non-DTM) HED endorsement feature, you need to manually add a HED tag first.

Also, some of these points are not realistically fixable for the release or are known by the EEGNet team and have a corresponding open issue. If any changes are requested, please formulate them as such.

@driusan driusan assigned kongtiaowang and charliehenrib and unassigned regisoc Nov 10, 2025
@christinerogers
Copy link
Contributor

Followup PR can include:

  • configurable HED Endorsement (useHEDendorsement Yes/No) since some LORISes may not use endorsement
  • maybe also configurable HED support/visibility ; since some LORISes may not want users to add tags (useHEDtags config; and/or permission to add, which would also need to be project-specific)
  • documentation on these features (e.g. module markdown, changelog)
  • note if above configs are added, help text displayed should also be governed by this config

@charliehenrib
Copy link
Contributor

charliehenrib commented Nov 11, 2025

  • Is it normal there are no all types?
image image
  • When is this button available? It's always greyed out for me.
image

@charliehenrib
Copy link
Contributor

@jeffersoncasimir Could you add some testing instructions please. It is the first time I am looking at this module and I don't really know where to go and what to test. Thank you

@jeffersoncasimir
Copy link
Contributor Author

jeffersoncasimir commented Nov 11, 2025

  • Is it normal there are no all types?

Yes. Thanks for pointing out that the help text should be revised. An "all" type appears if there is a "derivative" type. You would see raw | derivative | all, where either link only displays one and "all" would display both.

  • I think the link column can be removed from the csv download:

This is a valid issue, however I don't think it was introduced by this PR.

  • Broken link:

Noted. They recently moved the URL and set up redirection for seamlessness, but it was not done correctly (you can remove the www. and everything after .org to see redirection). I will modify to the new URL.

  • When is this button available? It's always greyed out for me.

It becomes available when you highlight a segment on the plot. This buttons changes the page bounds to this range.

Good catch. This is because the data is a symbolic link to raisinbread. I forgot that was the case on this VM. I will copy over the event files so that they are modifiable (required to download).

Screenshot 2025-11-11 at 9 51 06 AM

@jeffersoncasimir Could you add some testing instructions please. It is the first time I am looking at this module and I don't really know where to go and what to test. Thank you

The testing plan is a great starting point, and the help text has been expanded with a list of most features, but I will write something up that is tailored to this PR.

@jeffersoncasimir
Copy link
Contributor Author

@charliehenrib fyi: data has been copied over from raisinbread and should now be downloadable

@kongtiaowang
Copy link
Contributor

@jeffersoncasimir I found two small issues:
截屏2025-11-11 上午10 59 07

1,When a session has multiple EEGs, the brain map visualization is incorrect.

截屏2025-11-11 上午10 59 17

2,The HED link does not lead to a webpage.

@jeffersoncasimir
Copy link
Contributor Author

jeffersoncasimir commented Nov 11, 2025

1,When a session has multiple EEGs, the brain map visualization is incorrect.

Please use the raisinbread participants. This particular subject is experimental and just happens to be on my personal VM currently.

2,The HED link does not lead to a webpage.

This has been pointed out by the other two testers and is known.

@github-actions github-actions bot added Language: SQL PR or issue that update SQL code RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset Language: PHP PR or issue that update PHP code Language: Javascript PR or issue that update Javascript code labels Nov 11, 2025
@jeffersoncasimir
Copy link
Contributor Author

jeffersoncasimir commented Nov 11, 2025

@christinerogers @charliehenrib @kongtiaowang The HED resources URL has been modfiied.

@charliehenrib
Copy link
Contributor

Still getting an error in the background
image

@charliehenrib
Copy link
Contributor

THe error reported by Christine: tried to remove 2 tags added by Data authors. It showed me the X next to each orange tag (suggesting I could) so I clicked. then clicked Submit. got an error message but no details. The orange tags disappear and the submit button greyed out, but when i navigate elsewhere I am asked to confirm that i will lose changes. If i refresh the orange tags are back (delete failed) is still happening for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: EEG Issue or PR related to electroencephalography Critical to release PR or issue is key for the release to which it has been assigned Language: Javascript PR or issue that update Javascript code Language: PHP PR or issue that update PHP code Language: SQL PR or issue that update SQL code Module: electrophysiology_browser PR or issue related to electrophysiology_browser module Priority: High PR or issue should be prioritised over others for review and testing Project: EEGNet Issue or PR related to the EEGNet project RaisinBread PR or issue introducing/requiring improvements to the Raidinbread dataset Release: Add to release notes PR whose changes should be highlighted in the release notes Release: Document at release PR whose changes need to be documented in the wiki (or other documentation) at release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants