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.

Sign up for GitHub

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

Jump to bottom

[electrophysiology_browser] HED Tag Support #9033

Merged
merged 37 commits into from Mar 19, 2024
Merged

[electrophysiology_browser] HED Tag Support #9033

merged 37 commits into from Mar 19, 2024

Conversation

jeffersoncasimir
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir commented Jan 31, 2024 •
edited

This PR adds more robust HED tag support. Closes #8910.
Depends on #9032 (should be merged before reviewing)
Loris-MRI counterpart: #1041

It adds SQL tables to store HED schemas as hierarchical nodes and to store the assembled HED strings as node references using a modified linked list data structure, allowing for complex groupings.

Instance-level tags which were ingested in the HED column of events.tsv files can be viewed in the EventManager component and can be edited in a limited (see below) way in the AnnotationForm component.

There is a new component called DatasetTagger which is accessible via the "Dataset Tag Manager" button at the bottom left corner of the browser. This component enables the modification of HED tags at the dataset-wide level (events.json). This will associate HED tags to Event attributes for all events in the dataset containing the respective attributes. This component enables virtually unlimited groupings of HED tags, as well their addition and removal.

Limitations:

  • "Value" HED tags are not supported
  • "Expanded Definitions" are not supported
  • Editing the "Level description" via the "Dataset Tag Manager" is not currently supported (coming soon)
  • Session-level events.json overriding and assignment not currently supported.
  • Adding HED tags on an event level is currently limited to: SCORE artifacts (if SCORE schema ingested) and tags that were previously added via the ingestion of the events.json or manually via the Dataset Tag Manager.

TODO: Raisinbread modifications + HED tag inserts

MAKE SURE YOU ADD BOTH SCHEMAS WITH THE TOOL FIRST (or use RB): HED8.2.0.xml HED_score_1.1.0.xml

This was referenced Jan 31, 2024
Merged
Merged
@christinerogers christinerogers added Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) Caveat for Existing Projects PR contains changes that might impact the code or accepted practices of current active projects Document at Release PR adds or changes a feature such that the wiki (or other documentation) must be updated Critical to release PR or issue is key for the release to which it has been assigned Priority: High PR or issue should be prioritised over others for review and testing labels Feb 6, 2024
@driusan driusan added the Blocked PR awaiting the merge, resolution or rejection of an other Pull Request label Feb 6, 2024
@driusan
Copy link
Collaborator

driusan commented Feb 6, 2024

Blocked by #9032

@christinerogers christinerogers added this to the 26.0.0 milestone Feb 23, 2024
@jeffersoncasimir jeffersoncasimir dismissed driusan’s stale review February 29, 2024 15:18

Done. Clearing changes requested status

@laemtl
Copy link
Contributor

laemtl commented Feb 29, 2024

@jeffersoncasimir Can you rebase?

@christinerogers christinerogers added the Blocking PR should be prioritized because it is blocking the progress of another task label Feb 29, 2024
@christinerogers
Copy link
Contributor

added blocking because this is needed for release testing and for aces/Loris-MRI#1041

@jeffersoncasimir jeffersoncasimir removed the Blocking PR should be prioritized because it is blocking the progress of another task label Feb 29, 2024
driusan
driusan requested changes Mar 11, 2024
View reviewed changes
...s/electrophysiology_browser/jsx/react-series-data-viewer/src/eeglab/EEGLabSeriesProvider.tsx Outdated Show resolved Hide resolved
modules/electrophysiology_browser/jsx/react-series-data-viewer/src/series/components/Form.js Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/models/datasettags.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/models/datasettags.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/models/datasettags.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/models/datasettags.class.inc Outdated Show resolved Hide resolved
modules/electrophysiology_browser/php/models/datasettags.class.inc Show resolved Hide resolved
modules/electrophysiology_browser/php/models/electrophysioevents.class.inc Show resolved Hide resolved
@christinerogers christinerogers added the Blocking PR should be prioritized because it is blocking the progress of another task label Mar 12, 2024
@christinerogers
Copy link
Contributor

basically co-dependent with aces/Loris-MRI#1041 (blocked and blocking)

@jeffersoncasimir jeffersoncasimir mentioned this pull request Mar 13, 2024
Open
@jeffersoncasimir jeffersoncasimir dismissed driusan’s stale review March 14, 2024 15:26

Addressed. Updating status for overview

driusan
driusan reviewed Mar 19, 2024
View reviewed changes
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.

a couple minor non-blocking things

modules/electrophysiology_browser/php/models/electrophysiofile.class.inc
@@ -28,18 +28,22 @@ class ElectrophysioFile implements \LORIS\Data\DataInstance
private $_parameters = [];
private $_chunksURLs = [];
private $_splitFileIDs = [];
private \LORIS\LorisInstance $loris;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you probably want this to be protected, not private.

modules/electrophysiology_browser/php/models/electrophysiofile.class.inc
*/
function __construct(int $physiologicalFileID)
function __construct(\LORIS\LorisInstance $loris, int $physiologicalFileID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's easier to use PHP constructor property promotion if all you're doing is setting it directly:

https://stitcher.io/blog/constructor-promotion-in-php-8

@driusan driusan merged commit 659cc70 into aces:main Mar 19, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driusan driusan driusan left review comments

@christinerogers christinerogers christinerogers left review comments

@laemtl laemtl Awaiting requested review from laemtl

Assignees

@driusan driusan

Labels
Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) Blocking PR should be prioritized because it is blocking the progress of another task Caveat for Existing Projects PR contains changes that might impact the code or accepted practices of current active projects Critical to release PR or issue is key for the release to which it has been assigned Document at Release PR adds or changes a feature such that the wiki (or other documentation) must be updated Priority: High PR or issue should be prioritised over others for review and testing
Projects
None yet
Milestone
26.0.0
Development

Successfully merging this pull request may close these issues.

[EEG] HED Tag Support
4 participants
@jeffersoncasimir @driusan @laemtl @christinerogers