[New Feature] SQL Patch and functionality for EEG BIDS Derivatives #7345

AlexandraLivadas · 2021-02-22T17:53:04Z

This PR contains an SQL patch to create new tables for the annotation features that will be added in the Electrophysiology Browser module. This PR also includes basic functionalities in the EEG Browser to support these derivatives, including a filter in the main menu and a download option for annotation files.

To test this change

Run this patch and see that all statements work properly
Go to the main page and see that a filter has been added to filter between data that has derivatives and data that does not
a. Note that in order to test the filter, data will need to be added to the annotation_file table
Go to one of the session pages (raw or all types) and see that a "Download" button has been added for annotation files.
a. Note again that in order to test this button, data must be added to the annotation_file and annotation_archive tables. If needed, I can add a patch that will add data to these tables but this patch should be deleted before the PR is merged

Main Menu with Filter:

Sessions page with download annotations:

cmadjar

@AlexandraLivadas LGTM. See minor comments attached to the review.

cmadjar · 2021-02-23T22:12:04Z

SQL/New_patches/2021-02-19_annotation_tables.sql

+CREATE TABLE `annotation_instance` (
+    `AnnotationInstanceID` INT(10)      UNSIGNED NOT NULL AUTO_INCREMENT,
+    `AnnotationFileID`     INT(10)      UNSIGNED NOT NULL,
+    `ParameterFileID`      INT(10)      UNSIGNED NOT NULL,


Suggested change

`ParameterFileID` INT(10) UNSIGNED NOT NULL,

`AnnotationParameterID` INT(10) UNSIGNED NOT NULL,

keep the same column name as in the table annotation_instance.

cmadjar · 2021-02-23T22:13:17Z

SQL/New_patches/2021-02-19_annotation_tables.sql

+    `AbsoluteTime`         TIMESTAMP,
+    `Description`          VARCHAR(255),
+    PRIMARY KEY (`AnnotationInstanceID`),
+    CONSTRAINT `FK_parameter_file_ID`


Suggested change

CONSTRAINT `FK_parameter_file_ID`

CONSTRAINT `FK_annotation_parameter_ID`

cmadjar · 2021-02-23T22:13:32Z

SQL/New_patches/2021-02-19_annotation_tables.sql

+    `Description`          VARCHAR(255),
+    PRIMARY KEY (`AnnotationInstanceID`),
+    CONSTRAINT `FK_parameter_file_ID`
+        FOREIGN KEY (`ParameterFileID`)


Suggested change

FOREIGN KEY (`ParameterFileID`)

FOREIGN KEY (`AnnotationParameterID`)

cmadjar · 2021-02-23T22:15:16Z

SQL/New_patches/2021-02-19_annotation_tables.sql

+        REFERENCES `physiological_file` (`PhysiologicalFileID`),
+    CONSTRAINT `FK_annotation_file_type`
+        FOREIGN KEY (`FileType`)
+        REFERENCES `annotation_file_type` (`Type`)


Suggested change

REFERENCES `annotation_file_type` (`Type`)

REFERENCES `annotation_file_type` (`FileType`)

cmadjar · 2021-02-23T22:15:31Z

SQL/New_patches/2021-02-19_annotation_tables.sql

+
+-- Create annotation_file_type table
+CREATE TABLE `annotation_file_type` (
+    `Type`        VARCHAR(20)   NOT NULL UNIQUE,


Suggested change

`Type` VARCHAR(20) NOT NULL UNIQUE,

`FileType` VARCHAR(20) NOT NULL UNIQUE,

to be consistent with the FK in subsequent table

cmadjar

LGTM

christinerogers

Great additions Alex !

1 change requested on the "session" page : "Annotation Files" button rename and meove below events.
main module page: tweak the filter language and order, and also use the Links column to show when there is derived data (no new column needed)

modules/electrophysiology_browser/jsx/components/electrophysiology_session_panels.js

christinerogers

Hi Alex, thanks for these updates! it's coming along nicely

I've spotted a few more things to bring into better alignment with how 'Events' fields are consistently named.

Quick question for you and also @cmadjar 's input --:

Could these schema changes go in with the electrophysiology tables instead of a new schema file? (0000...6-Derivatives.sql) These tables include EEG field names hardcoded like Physiology_file_id, or channel in annotation_instance table.
Because they're presently very EEG-specific, I think it's better to name the tables as the Events tables are named: e.g. physiological_annotation_file instead of annotation_file ?

Remind me, was this considered?

Additional comments / tiny changes included inline below, please review.

christinerogers · 2021-03-17T21:06:20Z

modules/electrophysiology_browser/jsx/electrophysiologyBrowserIndex.js

+        type: 'select',
+        options: {
+          Raw: 'Raw',
+          Derivatives: 'Derivatives',


Suggested change

Derivatives: 'Derivatives',

Derivative: 'Derivative',

christinerogers · 2021-03-17T21:09:28Z

SQL/New_patches/2021-02-19_annotation_tables.sql

@@ -0,0 +1,117 @@
+-- SQL tables for BIDS derivative file structure


filename should change to ...electrophysiology_annotation_tables

christinerogers · 2021-03-17T21:10:00Z

modules/electrophysiology_browser/jsx/components/electrophysiology_session_panels.js

@@ -30,6 +30,7 @@ class FilePanel extends Component {
      'download_electrode_info',
      'download_channels_info',
      'download_events',
+      'download_annotation_files',


to match events:

Suggested change

'download_annotation_files',

'download_annotations',

christinerogers · 2021-03-17T21:10:25Z

modules/electrophysiology_browser/jsx/components/electrophysiology_session_panels.js

+                     target='_blank'
+                     download={this.state.data.downloads[0].file}
+                  >
+                    <button id='btn_download_annotation_files'


Suggested change

<button id='btn_download_annotation_files'

<button id='btn_download_annotations'

see above

christinerogers · 2021-03-17T21:10:38Z

modules/electrophysiology_browser/jsx/components/electrophysiology_session_panels.js

+                     style={stylesFile.div.element.download_title}
+                >Annotations</div>
+                <div className={'col-xs-2'}>
+                  <a id='download_annotation_files'


Suggested change

<a id='download_annotation_files'

<a id='download_annotations'

update to match

modules/electrophysiology_browser/jsx/electrophysiologyBrowserIndex.js

modules/electrophysiology_browser/jsx/electrophysiologySessionView.js

modules/electrophysiology_browser/php/sessions.class.inc

christinerogers · 2021-03-17T21:12:50Z

modules/electrophysiology_browser/test/electrophysiologyBrowserTest.php

@@ -268,7 +268,8 @@ function testEEGBrowserWithSitePermissions()
     */
    function testFilters()
    {
-        $this->safeGet($this->url . "/electrophysiology_browser/");
+        $this->markTestSkipped('Must be updated with annotation additions to pass');


Suggested change

$this->markTestSkipped('Must be updated with annotation additions to pass');

$this->markTestSkipped('Must be updated with annotations to pass');

is this suggestion here more accurate?

christinerogers · 2021-03-17T21:13:11Z

modules/electrophysiology_browser/test/electrophysiologyBrowserTest.php

@@ -342,6 +343,7 @@ function testFilters()
     */
    function testEEGBrowserSortableByColumn()
    {
+        $this->markTestSkipped('Must be updated with annotation additions to pass');


same : "annotations" is perhaps clearer than "annotation additions" ?

more instances of the same below...

AlexandraLivadas · 2021-03-18T19:43:59Z

@christinerogers To respond to your latest comments/requested changes, I think putting the SQL changes in the same schema as the EEG (physiological) tables does make sense in this case. You're right that these tables are only related to physiological files and cannot be applied to other types of files or modules, so it makes sense to rename the tables to make that relation clearer. I've made the requested changes :)

laemtl · 2021-03-19T16:23:36Z

modules/electrophysiology_browser/php/electrophysiologybrowserrowprovisioner.class.inc

              s.ID                                      AS SessionID,
              s.CenterID                                AS CenterID,
-              s.ProjectID                               AS ProjectID
+              s.ProjectID                               AS ProjectID              


Unnecessary white spaces

laemtl · 2021-03-19T21:42:50Z

modules/electrophysiology_browser/php/electrophysiologybrowserrowprovisioner.class.inc

@@ -47,10 +47,17 @@ class ElectrophysiologyBrowserRowProvisioner
              s.Visit_label                             AS Visit_Label,
              MIN(pf.AcquisitionTime)                   AS Acquisition_Time,
              MIN(pf.InsertTime)                        AS Insert_Time,
-              GROUP_CONCAT(DISTINCT pot.OutputTypeName) AS Links,
+              GROUP_CONCAT(DISTINCT CASE 
+                WHEN pot.OutputTypeName='derivatives' THEN 'raw,derivative'


Should be WHEN pot.OutputTypeName='derivatives' THEN 'raw,derivatives' otherwise the type is unrecognized (sessions.inc.class only accept raw and derivatives as valid types).

Christine requested that I change all "derivatives" to singular. Is it alright if I change sessions.class.inc to take "raw" or "derivative" as valid output types instead to match?

I tend to think that renaming the fields in the query is prone to mistakes as it introduces mismatches between the backend/frontend and the database naming convention. Ideally, the database entry should match but I don't know if changing it at this point is a good idea.

laemtl · 2021-03-19T21:51:39Z

modules/electrophysiology_browser/php/electrophysiologybrowserrowprovisioner.class.inc

+                ELSE 'raw'
+              END) AS Links,
+              GROUP_CONCAT(DISTINCT CASE 
+                WHEN pot.OutputTypeName='derivatives' THEN 'Derivative'


Should be WHEN pot.OutputTypeName='derivatives' THEN 'Derivatives' to be consistent with the row entries (since the recommended change above).

If you look at Christine's requested changes above, she requested that I change all mentions of "Derivatives" to singular and not plural

Therefore the sessions class needs an update (and ideally the database entry: SQL/0000-00-05-ElectrophysiologyTables.sql).

laemtl · 2021-03-19T21:56:31Z

modules/electrophysiology_browser/jsx/electrophysiologyBrowserIndex.js

+        name: 'type',
+        type: 'select',
+        options: {
+          Raw: 'Raw',


Might be more flexible to pass the options as props to stay consistent with the database state. (see how this is done for Site, Project). That way, if a new element is added in the table physiological_output_type, it will automatically be added in the filter.

Good point!

laemtl · 2021-03-19T22:57:33Z

modules/electrophysiology_browser/php/electrophysiologybrowserrowprovisioner.class.inc

-              GROUP_CONCAT(DISTINCT pot.OutputTypeName) AS Links,
+              GROUP_CONCAT(DISTINCT CASE 
+                WHEN pot.OutputTypeName='derivatives' THEN 'raw,derivative'
+                ELSE 'raw'


This works until a new type is added in OutputTypeName. It will fall in the else case and be assigned to 'raw'.
It's safer to change for ELSE pot.OutputTypeName

laemtl · 2021-03-19T23:10:08Z

modules/electrophysiology_browser/php/electrophysiologybrowserrowprovisioner.class.inc

+              END) AS Links,
+              GROUP_CONCAT(DISTINCT CASE 
+                WHEN pot.OutputTypeName='derivatives' THEN 'Derivative'
+                ELSE 'Raw'


Same comment here... this works until a new type is added in OutputTypeName. It will fall in the else case and be assigned to 'Raw'.

laemtl · 2021-03-19T23:19:41Z

modules/electrophysiology_browser/php/electrophysiologybrowserrowprovisioner.class.inc

@@ -47,10 +47,17 @@ class ElectrophysiologyBrowserRowProvisioner
              s.Visit_label                             AS Visit_Label,
              MIN(pf.AcquisitionTime)                   AS Acquisition_Time,
              MIN(pf.InsertTime)                        AS Insert_Time,
-              GROUP_CONCAT(DISTINCT pot.OutputTypeName) AS Links,


The GROUP_CONCAT/DISTINCT combo prevents duplicates entries when a session contains multiple files of the same type (ex: 2 raw files will display only one raw link in the data table). For 2 files of mixed type, it will display raw twice in the links in the data table (since ''raw", and ''raw,derivative" are distinct).

This issue has been resolved in the latest commit, and the picture in the PR description has been updated to show the new view!

laemtl · 2021-03-20T00:21:34Z

I noticed the filter Type stopped working for entries containing multiple files:
In this case, type is a string containing all the values: 'derivative,raw', which doesn't match either derivative or raw.

{
"Site": "Ottawa",
"PSCID": "OTT174",
"DCCID": "300174",
"Project": "Pumpernickel",
"Visit Label": "V1",
"Acquisition Time": "2019-06-19 09:17:05",
"Insertion Time": "2019-08-21 15:10:13",
"Links": "derivative,raw",
"Type": "derivative,raw",
"SessionID": "174"
}

christinerogers · 2021-03-31T15:06:46Z

@AlexandraLivadas how are you doing with these comments? Stuck anywhere?

laemtl · 2021-04-07T01:30:07Z

SQL/0000-00-05-ElectrophysiologyTables.sql

+
+-- Create annotation_archive which will store archives of all the annotation files for
+-- Front-end download
+CREATE TABLE `physiological_annotation_archive` (


TO DISCUSS - If the annotations need to be synchronized with user inputs, this table should maybe be deleted and the zip created on the fly when download is requested.

This is a good point. This is the way that the pysiological files are handled in general, which is why I implemented it this way. You're right though that in this case, it does not make sense.

For now, let's follow the example of the .tgz created by the pipeline specifically as a downloadable package.
e.g. bids_imports/Face13_BIDSVersion_1.1.0/sub-OTT166/ses-V1/eeg/sub-OTT166_ses-V1_task-faceO_eeg.tgz
Basically the archive for annotations should be created/structured similarly.

Agreed this should be revisited and discussed when we come to adding/editing annotations in the module - e.g. are all .tgz updated?

Ok in that case I will leave this table in, as that is how it's structured for the physiological tables/files.

christinerogers

nice work, @AlexandraLivadas

@driusan Could you please final review?

It's blocking another time-sensitive PR, thanks.

laemtl · 2021-04-22T19:43:12Z

@AlexandraLivadas
Tests are failing since the last changes. Can you rebase and see if that helps? Otherwise, I suspect a MySQL problem.

laemtl · 2021-04-23T01:50:46Z

@driusan The PR is now passing the tests.

christinerogers · 2021-04-23T14:20:33Z

Thanks @AlexandraLivadas and @laemtl
Ready for final review - @cmadjar I think this is good to go, but in case you want to re-review.

@driusan if you have time - thanks for looking at it when you can, this is blocking a few other PRs which are important for a mid-May grant deliverable.

Re-enable and fix skipped tests in PR #7345.

…ces#7345) This contains an SQL patch to create new tables for the annotation features that will be added in the Electrophysiology Browser module. It also includes basic functionalities in the EEG Browser to support these derivatives, including a filter in the main menu and a download option for annotation files.

Re-enable and fix skipped tests in PR aces#7345.

AlexandraLivadas added Feature PR or issue introducing/requiring at least one new feature SQL PR contains SQL modifications such as schema changes and new SQL scripts labels Feb 22, 2021

AlexandraLivadas changed the title ~~[New Feature] SQL Patch for BIDS Derivatives~~ [New Feature] SQL Patch for EEG Browser Derivatives Feb 22, 2021

AlexandraLivadas changed the title ~~[New Feature] SQL Patch for EEG Browser Derivatives~~ [New Feature] SQL Patch for Electrophysiology Browser Derivatives Feb 22, 2021

AlexandraLivadas changed the title ~~[New Feature] SQL Patch for Electrophysiology Browser Derivatives~~ [New Feature] SQL Patch for Electrophysiology Derivatives Feb 22, 2021

AlexandraLivadas changed the title ~~[New Feature] SQL Patch for Electrophysiology Derivatives~~ [New Feature] SQL Patch for EEG BIDS Derivatives Feb 22, 2021

cmadjar requested changes Feb 23, 2021

View reviewed changes

laemtl mentioned this pull request Mar 12, 2021

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

Merged

cmadjar approved these changes Mar 12, 2021

View reviewed changes

AlexandraLivadas changed the title ~~[New Feature] SQL Patch for EEG BIDS Derivatives~~ [New Feature] SQL Patch and functionality for EEG BIDS Derivatives Mar 12, 2021

christinerogers requested changes Mar 15, 2021

View reviewed changes

modules/electrophysiology_browser/jsx/components/electrophysiology_session_panels.js Outdated Show resolved Hide resolved

christinerogers requested a review from laemtl March 15, 2021 19:24

AlexandraLivadas marked this pull request as ready for review March 17, 2021 18:22

AlexandraLivadas requested review from cmadjar and christinerogers and removed request for cmadjar March 17, 2021 18:43

christinerogers requested changes Mar 17, 2021

View reviewed changes

AlexandraLivadas requested a review from christinerogers March 18, 2021 19:44

laemtl reviewed Mar 19, 2021

View reviewed changes

laemtl requested changes Mar 19, 2021

View reviewed changes

laemtl reviewed Mar 19, 2021

View reviewed changes

laemtl requested changes Mar 19, 2021

View reviewed changes

christinerogers approved these changes Apr 6, 2021

View reviewed changes

laemtl reviewed Apr 7, 2021

View reviewed changes

christinerogers approved these changes Apr 21, 2021

View reviewed changes

AlexandraLivadas added 17 commits April 22, 2021 16:45

[New Feature] SQL Patch for BIDS Derivatives

82c7041

Removed DROP TABLES

87b608f

Requested changes and default schema

fd6dcd0

Renamed file

2b81601

Small changes made to tables, typo removed

c02db80

Added filter for raw or derived

bd762c6

Download functionality, small changes to SQL

e026724

Formatting errors resolved

f37c622

Formatting error

c16205a

Requested changes made

c0b322b

Changes to table and component naming

0ea9e20

Requested changes and filter fixed

8d51308

Changes to annotation instance table

fe3dc55

Edited links list

bcf371e

Removed drop tables from new patch

46f409e

Small edit

d95a3ad

Changed duration to decimal

95fcf5d

AlexandraLivadas force-pushed the 2021-02-19_annotation_sql_tables branch from 316cbb1 to 95fcf5d Compare April 22, 2021 21:19

Fix MySQL syntax error

5452dfd

AlexandraLivadas mentioned this pull request Apr 26, 2021

[EEG] Add view/edit functionality and permissions for EEG BIDS derivatives #7433

Merged

Fixed mySQL syntax error

69d4901

driusan merged commit b6c59bb into aces:main Apr 27, 2021

driusan pushed a commit that referenced this pull request Apr 28, 2021

[EEG Browser] Integration fix (#7436)

60575c7

Re-enable and fix skipped tests in PR #7345.

AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021

[EEG Browser] Integration fix (aces#7436)

119e4b5

Re-enable and fix skipped tests in PR aces#7345.

ridz1208 added this to the 24.0.0 milestone Aug 24, 2021

	`ParameterFileID` INT(10) UNSIGNED NOT NULL,
	`AnnotationParameterID` INT(10) UNSIGNED NOT NULL,

	CONSTRAINT `FK_parameter_file_ID`
	CONSTRAINT `FK_annotation_parameter_ID`

	FOREIGN KEY (`ParameterFileID`)
	FOREIGN KEY (`AnnotationParameterID`)

	REFERENCES `annotation_file_type` (`Type`)
	REFERENCES `annotation_file_type` (`FileType`)

	`Type` VARCHAR(20) NOT NULL UNIQUE,
	`FileType` VARCHAR(20) NOT NULL UNIQUE,

		@@ -0,0 +1,117 @@
		-- SQL tables for BIDS derivative file structure

	<button id='btn_download_annotation_files'
	<button id='btn_download_annotations'

	<a id='download_annotation_files'
	<a id='download_annotations'

	$this->markTestSkipped('Must be updated with annotation additions to pass');
	$this->markTestSkipped('Must be updated with annotations to pass');

[New Feature] SQL Patch and functionality for EEG BIDS Derivatives #7345

[New Feature] SQL Patch and functionality for EEG BIDS Derivatives #7345

Conversation

AlexandraLivadas commented Feb 22, 2021 • edited Loading

To test this change

cmadjar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar left a comment

Choose a reason for hiding this comment

christinerogers left a comment • edited Loading

Choose a reason for hiding this comment

christinerogers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christinerogers Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandraLivadas commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laemtl Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laemtl Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

laemtl Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laemtl Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laemtl Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laemtl commented Mar 20, 2021 • edited Loading

christinerogers commented Mar 31, 2021

laemtl Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christinerogers left a comment

Choose a reason for hiding this comment

laemtl commented Apr 22, 2021

laemtl commented Apr 23, 2021

christinerogers commented Apr 23, 2021 • edited Loading

AlexandraLivadas commented Feb 22, 2021 •

edited

Loading

christinerogers left a comment •

edited

Loading

christinerogers Mar 17, 2021 •

edited

Loading

laemtl Mar 19, 2021 •

edited

Loading

laemtl Mar 19, 2021 •

edited

Loading

laemtl Mar 19, 2021 •

edited

Loading

laemtl Mar 19, 2021 •

edited

Loading

laemtl Mar 19, 2021 •

edited

Loading

laemtl commented Mar 20, 2021 •

edited

Loading

laemtl Apr 7, 2021 •

edited

Loading

christinerogers commented Apr 23, 2021 •

edited

Loading