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-BIDS] Add support for events.json file & HED Tags #769

Merged
merged 23 commits into from
Jun 28, 2022

Conversation

jesscall
Copy link
Contributor

@jesscall jesscall commented Mar 9, 2022

Add support to bids_import for *events.json file & HED Tags

Scope

This PR refactors how the bids_import handles event files. It adds support for an event.json file in which HED tags can be stored. The PR edits the fetching and inserting of event files and adds an archiving feature similar to the annotation files.

Information

Please reference this documentation for more info on HED Tags in BIDS

Testing

  1. The SQL patches from [EEG] Database Architecture for HED Tags Loris#8036 should be sourced before attempting to test these changes.
  2. Place the sample *events.json file at the root of the RB BIDS directory.
  3. Attempt to test the changes in this PR
  4. Try putting the *events.json file in a subject directory (and renaming) and make sure the import still runs as expected.

@jesscall jesscall added A-BIDS Area: BIDS. Issues and pull requests related to BIDS and the BIDS import pipeline A-EEG Area: EEG. Issues and pull requests related to EEG / iEEG processing labels Mar 9, 2022
@jesscall
Copy link
Contributor Author

@cmadjar I made the database_lib additions - I haven't had a chance to re-test the import however. I'll try to get to this by end of week

@cmadjar cmadjar added this to the 25.0.0 milestone Mar 16, 2022
@jesscall
Copy link
Contributor Author

Bug:

  • The events.json is being stored at the individual candidate level. Should be stored only once at the BIDS root level... @cmadjar I haven't had the chance to debug this yet. Any ideas off the top of your head how I can achieve this since you are more familiar with the code?

Also to note:
There will be some more changes coming to this PR to account for HED tag assembly.

@jesscall
Copy link
Contributor Author

jesscall commented Jun 2, 2022

This PR now supports assembly of events' HED tags using the HEDtools web service.

python/lib/physiological.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

@jesscall thank you for working on this!!

I tested the following:

  • no events.json anywhere => 👍
  • task-faceO_events.json at the root directory of the BIDS dataset => for some reason, the content of physiological_event_parameter_category does not populate Description and HED fields
12	2	boundary	NULL	NULL
13	2	checker-left	NULL	NULL
14	2	checker-right	NULL	NULL
15	2	e-254	NULL	NULL
16	2	e-255	NULL	NULL
17	2	face-inverted	NULL	NULL
18	2	face-upright	NULL	NULL
19	2	house-inverted	NULL	NULL
20	2	house-upright	NULL	NULL
21	2	press-left	NULL	NULL
22	2	press-right	NULL	NULL

I would have expected to see Description="boundary event" and HED="Experiment-procedure" for LevelName="boundary"

  • sub-OTT501_task-faceO_events.json in the subject directory => tables physiological_event_parameter and physiological_event_parameter_category do not get populated for some reason (physiological_event_file, physiological_event_archive and physiological_task_event get populated though)

File used for testing: the file you sent me on Slack last Friday.

python/lib/database_lib/physiologicaleventfile.py Outdated Show resolved Hide resolved

return event_paths

def grep_event_file_id_from_event_path(self, event_file_path):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is event_file_path supposed to be the TSV file?

Also, what would happen if an event file is common to all physiological files? Maybe the wrong eventFileID could be returned?

Example:

EventFileID     PhysiologicalFileID     FileType    FilePath
1               1                       tsv         <BIDS_ROOT_DIR>/task-faceO_events.tsv
2               1                       json        <BIDS_ROOT_DIR>/task-faceO_events.json
3               2                       tsv         <BIDS_ROOT_DIR>/task-faceO_events.tsv
4               2                       json        <BIDS_ROOT_DIR>/task-faceO_events.json

looking at the return of the function, it looks like it would return EventFileID = 1 (for TSV) or 2 (for JSON) which are linked to PhysiologicalFileID 1 even though this is PhysiologicalFileID 2 that is under process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it takes in the event_tsv file

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok. But what happens in the example above where the same TSV file links to two different PhysiologicalFileID? Would it just return the first PhysiologicalFileID encountered? If so, sounds like a possible major bug, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where do you see the case where two tsv point to the same physioFile?

Copy link
Collaborator

Choose a reason for hiding this comment

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

when the event.tsv file is identical for all EEG files and therefore put directly under root. I believe this is possible in BIDS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The event.json file can be shared by all subjects. I don't think the events.tsv can ? This means that ALL recordings have the same events/ stimulus? Seems unlikely, but I could be wrong. Do you have an example of this in the spec somewhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

From the BIDS specs: https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/05-task-events.html

It is also possible to have a single events.tsv file describing events for all participants and runs (see [Inheritance Principle](https://bids-specification.readthedocs.io/en/stable/02-common-principles.html#the-inheritance-principle)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by passing in physioFileID as a param in the query

python/lib/database_lib/physiologicaleventparameter.py Outdated Show resolved Hide resolved
python/lib/eeg.py Outdated Show resolved Hide resolved
Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca>
python/lib/physiological.py Outdated Show resolved Hide resolved
@jesscall
Copy link
Contributor Author

@cmadjar I believe this commit af956f7 will address the issue with physiological_event_parameter_category_level saving

python/lib/eeg.py Outdated Show resolved Hide resolved
python/lib/eeg.py Outdated Show resolved Hide resolved
python/lib/eeg.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

@jesscall see replies to your comments.

Also, once the LORIS side has been changed to link the file types to ImagingFileTypes, a little reminder to modify the code here to reflect on the DB structure


return event_paths

def grep_event_file_id_from_event_path(self, event_file_path):
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok. But what happens in the example above where the same TSV file links to two different PhysiologicalFileID? Would it just return the first PhysiologicalFileID encountered? If so, sounds like a possible major bug, no?

python/lib/eeg.py Outdated Show resolved Hide resolved
Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca>
python/lib/eeg.py Outdated Show resolved Hide resolved
jesscall and others added 2 commits June 20, 2022 13:43
Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca>
Copy link
Collaborator

@cmadjar cmadjar left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Waiting for aces/Loris#8036 to be merged on the LORIS to merge this PR.

@cmadjar cmadjar added the Blocked Merge it and you die label Jun 28, 2022
@cmadjar cmadjar merged commit c1bbcbd into aces:main Jun 28, 2022
zaliqarosli pushed a commit to zaliqarosli/Loris-MRI that referenced this pull request Mar 9, 2023
* refactoring how bids_import handles event files: fetch, insert, archiving of tsv & json

* bugfixes!

* add event tables to database_lib

* lint

* missed one

* ?

* some more fixes after testing import with db libs

* hed-assemble events

* lint

* more lint edits

* rm prints

* rm print

* fixed bug with storing events.json and naming of archive

* Apply suggestions from code review

Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca>

* Fix values for save to parameter_category_level

* lint

* trying to fix flake8

* flake 8?

Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca>

* Update python/lib/eeg.py

Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca>

* fixing grwp eventFileID function

* fix EOL error

* keep making syntax errors :(

Co-authored-by: Cécile Madjar <cecile.madjar@mcin.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-BIDS Area: BIDS. Issues and pull requests related to BIDS and the BIDS import pipeline A-EEG Area: EEG. Issues and pull requests related to EEG / iEEG processing Blocked Merge it and you die
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants