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] Task to property prefix for additional events table #8432

Merged

Conversation

regisoc
Copy link
Contributor

@regisoc regisoc commented Mar 8, 2023

Brief summary of changes

Rename the additional events' table attributes prefix from Task to Property to match BIDS naming.

Linked to LORIS-MRI 900

@regisoc regisoc self-assigned this Mar 8, 2023
@regisoc regisoc added the Language: SQL PR or issue that update SQL code label Mar 8, 2023
@regisoc regisoc changed the title task to property prefix for additional events table [EEG] Task to property prefix for additional events table Mar 8, 2023
@christinerogers
Copy link
Contributor

christinerogers commented Mar 8, 2023

This could technically be normalized further since PropertyName is a column name in an events.tsv file (and in the json etc), right?
Multiple events will have the same PropertyName (with each their own PropertyValue)
not sure if it's been considered / worth it @laemtl

@laemtl laemtl added this to the 25.0.0 milestone Mar 8, 2023
@@ -3,8 +3,8 @@
CREATE TABLE `physiological_task_event_opt` (
Copy link
Contributor

@christinerogers christinerogers Mar 8, 2023

Choose a reason for hiding this comment

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

the name of the table should change too @regisoc @laemtl @jeffersoncasimir yes?
literally physiological_event_additional would be more accurate I think

Copy link
Contributor

Choose a reason for hiding this comment

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

I am hesitant on that one since the original table name for event is physiological_task_event (which started the confusion :))

Copy link
Contributor

@laemtl laemtl Mar 8, 2023

Choose a reason for hiding this comment

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

See BIDS doc, where they also refer to task events

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree that is the source of the confusion :)

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.

@jeffersoncasimir let me know if i missed something in flagging these, thanks

@christinerogers christinerogers self-requested a review March 8, 2023 20:49
@laemtl
Copy link
Contributor

laemtl commented Mar 8, 2023

This could technically be normalized further since PropertyName is a column name in an events.tsv file (and in the json etc), right? Multiple events will have the same PropertyName (with each their own PropertyValue) not sure if it's been considered / worth it @laemtl

This has been discussed and I think I agree that this would be a better approach for extensibility/future improvements.
@regisoc any thoughts?

@regisoc
Copy link
Contributor Author

regisoc commented Mar 8, 2023

We discussed this and yes, this approach allows for any potential additional property.
There will be repetition for each PhysiologicalTaskEventID (if I remember correctly, every line in _event.tsv file) only if the property is initialized. So, if there is very few data on additional columns, that is taken care off.
I think this is the most extensive and permissive way because we can add any new column name in _event.tsv file as a single line in this table, which translate with (task event ID, name of the additional property for this task event, value of the additional property for this task event).

@christinerogers christinerogers removed their request for review March 9, 2023 22:14
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.

lgtm with Laetitia and Jefferson's blessing. thanks @regisoc for being on top of this so quickly

@christinerogers
Copy link
Contributor

@driusan i think this is ready to merge.
cc @laemtl

@christinerogers christinerogers requested a review from laemtl March 9, 2023 22:16
@driusan driusan merged commit 79a8e40 into aces:main Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language: SQL PR or issue that update SQL code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants