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

update hed description to clarify trial_type #33

Closed
wants to merge 6 commits into from
Closed

update hed description to clarify trial_type #33

wants to merge 6 commits into from

Conversation

VisLab
Copy link
Member

@VisLab VisLab commented Oct 4, 2018

Still trying to clarify tags associated with general classes of events and those associated with event instances.

@VisLab
update hed description to clarify trial_type
147dc0b 
Still trying to clarify tags associated with general classes of events and those associated with event instances.
@chrisgorgo
Copy link
Contributor

This looks good to me. To clarify - does this PR replace #26?

@VisLab
Copy link
Member Author

VisLab commented Oct 5, 2018

Yes, this is a replacement for #26

@sappelhoff
Copy link
Member

However, as explained in the section above, trial-types have HED tags stored in a map

Could somebody point me to this "map" or the trial_type dictionary as you name it in this PR? I can't find anything on this in the specification. @chrisfilo can you help out?

and even if there is such a map ... who updates this map? As long as there are no clear rules for the trial_type column, people can come up with whatever description they deem sensible (which is good!). The map would thus become rather hard (read: impossible) to maintain ... and on the other hand - if we had strict rules for the trial_type column, we might as well just use the HED rules.

@VisLab
Copy link
Member Author

VisLab commented Oct 5, 2018 via email

@sappelhoff
Copy link
Member

link for convenience: HED appendix

Okay, thanks for the explanation! I think I see what you mean but I am not sure how it will work.

See this events.tsv file ... I have added a column actual_event just for the sake of the example ... imagine it was not actually present in the file.

onset    duration    trial_type    *actual_event*
1.1    n/a    go    fixation cross onset
1.3    n/a    go    stimulus onset left of fix cross
1.5    n/a    go    participant reaction to stimulus
2.1    n/a    no-go    fixation cross onset
2.3    n/a    no-go    stimulus onset left of fix cross
...

Now with your approach, we would provide an events.json file with a key trial_type that has as its value a json object {}.

One key of this trial_type json object is HED, which is containing a mapping between each trial_type and an HED tag:

{
  "trial_type": {
    "HED": {
      "go": "some/HED/tag; that/goes/like/this",
      "no-go": "some/HED/tag; that/goes/differently",
    }
  }
}

Did I understand you correctly up to this point? If yes, there are two questions that I have:

  1. Concretely: How would you define the HED tags in the present example (i.e., please replace some/HED/tag; that/goes/like/this by the actual HED tag one should use in such a case)
  2. What would you do if this mapping was not supplied by the dataset curator ... i.e., no "HED" key in the events.json, or even no events.json at all?

sappelhoff
sappelhoff requested changes Oct 5, 2018
View reviewed changes
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

needs clarification before merging (see comments in conversation)

@VisLab
Copy link
Member Author

VisLab commented Oct 5, 2018 via email

@sappelhoff
Copy link
Member

sappelhoff commented Oct 5, 2018 •
edited
Loading

Thanks for providing the example tags! Of course these are minimal because the example is constraint, but they give a good idea of what it would be like to use HED tags here.

You need to have the facility to have users HED tag event codes as well as individual event instances.

I don't understand what you mean by that. Are you saying that a single column "HED" is not sufficient to exhaustively describe an event?

I think you need to have maps for both trial_type and actual_event.

Sorry, but I don't think I can follow this reasoning. I think we have now clarified that the trial_type column is truly separate from the HED column. Just think of it as two ways to describe events.

  • The trial_type is arguably very limited and idiosyncratic ... but quick and simple. (imagine an experiment where a participant lifts a hand during a trial)
onset    duration    trial_type
1.1    n/a    left
2.3    n/a    left
4.5    n/a    right
...

In the accompanying events.json, we can have the following map:

{
    "trial_type": {
        "LongName": "trial type",
        "Description": "Which hand was lifted by the participant.",
        "Levels": {
            "left": "Left hand was lifted",
            "right": "Right hand was lifted"
        }
}

now completely separate from this, you can describe each event using the HED tag column ... it's also possible not to use trial_type at all. The HED column allows for a standardized and exhaustive description of the event, albeit at the cost that it is harder to do and there is currently no inbuilt validator (see this issue: https://github.com/bids-standard/bids-validator/issues/537). Note that for the HED column, a "map" in the events.json is not really necessary, because all HED tags are already standardized and defined - there is no room for interpretation (which is the big plus over the simple trial_type column), and with no room for interpretation there is no need to document in the events.json. Instead, users can simply refer to the HED tag documentation at http://www.hedtags.org/ or similar.

With these premises, could you try to explain what the problem is?

@VisLab
Copy link
Member Author

VisLab commented Oct 8, 2018 via email

@chrisgorgo
Copy link
Contributor

@sappelhoff let me try to explain what the problem is

Imagine the following scenario

_events.tsv

onset    duration    trial_type HED
1.1    1    left  HedTagA
2.3    1    left  HedTagB
4.5    1    right HedTagC

Accompanied by the following _events.json

{
    "trial_type": {
        "HED": {
            "left": "HedTagB",
            "right": "HedTagD"
        }
}

Currently the spec is unclear how the information about HED tags from _events.tsv and _events.json should be combined. The proposal in this PR states that a sum of two sets should be taken therefore:

  • Event 1 HED tags: HedTagA and HedTagB
  • Event 2 HED tags: HedTagB
  • Event 3 HED tags: HedTagC and HedTagD

Does this make more sense now?

@VisLab
Copy link
Member Author

VisLab commented Oct 8, 2018 via email

@chrisgorgo
Copy link
Contributor

I think spec needs clarification and this PR provides it. I just added an example to the discussion to help @sappelhoff understand the context.

@VisLab
Copy link
Member Author

VisLab commented Oct 8, 2018 via email

@chrisgorgo
Copy link
Contributor

I'm not seeing any new examples on the spec..... could you point me in the
right direction?

When I mentioned "examples" I was referring to the one I posted in this thread as a comment (#33 (comment)). If you think it would be useful to add this to the spec please update the pull request (https://egghead.io/lessons/javascript-how-to-update-a-pull-request-on-github).

I would like there to be event.json files allowed at higher levels in the
BIDS file structure that have these maps that apply to lower levels in the
hierarchy. The typical use case is that the experimenter specifies one
events.json file at the top level that specifies the event classes (again
--- it doesn't have to be a single event-type as you could specify for
example two or three of these (say event_type and trial_type) each with
HED tag maps. The recording specific xxx_event.tsv could have columns
with some or all of event_type and trial_type and HED and all that are
specified would be used.

This is already allowed since the inheritance principle applies to events.tsv and events.json. If you think it's useful it might be worth fleshing this out in the context of HED and "global tags" in the HED appendix.

@sappelhoff
Copy link
Member

Thanks for providing the example @chrisfilo. I understood the point now - adding the mentioned example to the spec would be a valuable contribution.

Regarding @VisLab's previous comment:

The statement "Note that for the HED column, a "map" in the events.json is not really possible, because all HED tags are already standardized and defined - there is no room for interpretation (which is the big plus over the simple trial_type column), " doesn't make any sense to me.

replace "not really possible" with "not really necessary"

@VisLab
Copy link
Member Author

VisLab commented Oct 8, 2018

Sorry, I was reading these threads from my email rather than github and the comments got out of sync. I will make a first pass at a clarifying explanation of global tags. I agree the HED column shouldn't have a map. It is designed for instance-specific tags.

@VisLab
Changed trial_type to event_type in example
40a8619 
I think event_type is a better name for the particular items used in the example.  We could add a second data dictionary with trial_type in with something like left button or right button.
@VisLab
Copy link
Member Author

VisLab commented Oct 8, 2018

I added something on current branch. However, in doing so, I realized that I don't completely understand the BIDS naming convention. Could someone clarify the naming convention for _events.json files at higher levels in the hierarchy. Thanks....

chrisgorgo
chrisgorgo reviewed Oct 8, 2018
View reviewed changes
src/99_appendices/03_hed.md Outdated

Example:
```JSON
{
"trial_type": {
"evebt_type": {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

chrisgorgo
chrisgorgo reviewed Oct 8, 2018
View reviewed changes
src/99_appendices/03_hed.md
1.2 0.6 fixationCross Event/Category/Experimental stimulus, Event/Label/CrossFix, Event/Description/A cross appears at screen center to serve as a fixation point, Sensory presentation/Visual, Item/Object/2D Shape/Cross, Attribute/Visual/Fixation point, Attribute/Visual/Rendering type/Screen, Attribute/Location/Screen/Center
5.6 0.008 target Event/Label/Target image, Event/Description/A white airplane as the RSVP target superimposed on a satellite image is displayed., Event/Category/Experimental stimulus, (Item/Object/Vehicle/Aircraft/Airplane, Participant/Effect/Cognitive/Target, Sensory presentation/Visual/Rendering type/Screen/2D), (Item/Natural scene/Arial/Satellite, Sensory presentation/Visual/Rendering type/Screen/2D)
500 0.008 nontarget Event/Label/Non-target image, Event/Description/A non-target image is displayed for about 8 milliseconds, Event/Category/Experimental stimulus, (Item/Natural scene/Arial/Satellite, Participant/Effect/Cognitive/Expected/Non-target, Sensory presentation/Visual/Rendering type/Screen/2D), Attribute/Onset
```

Alternatively if the same HED tags apply to a group of events with the same `trial_type` they can be specified in the corresponding data dictionary (`_events.json` file) using the following syntax:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the need to introduce a new prescribed column name. Why not use trial_type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry evebt_type was a typo. I think that trial_type is confusing --- there are usually several events in a trial (e.g., stimulus and response). The example given of trial_type was of an experimental condition rather than something that would naturally correspond to an "event code" that would be coming off of an acquisition system. I think it would be natural to have both.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest opening a separate PR proposing the addition of event_type - it's going to be a much more controversial (it is potentially backward incompatible, it requires a clear definition in https://github.com/bids-standard/bids-specification/blob/master/src/04-modality-specific-files/03-task-events.md). The issue of combining tags between JSON and TSV is much more straightforward and consensus will be easier to reach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does trial_type have some special meaning in BIDS? What I was proposing that the user could add any columns that they wanted to ( --- may I should call the column giraffes in the example?) and the idea would be that HED maps that were defined at various levels would be applied if they matched column names. I didn't mean to conflict with a BIDS standardized column. It is not meant to be a prescribed column....

Copy link
Contributor

Choose a reason for hiding this comment

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

trial_type is defined here: https://github.com/bids-standard/bids-specification/blob/master/src/04-modality-specific-files/03-task-events.md. I see your point and I guess this uncovers another lack of clarity in the current description - can one annotate any column from events.tsv with HED tags in events.json or only trial_type? If the answer is any column this needs to be clarified and could be part of this PR.

@chrisgorgo
Copy link
Contributor

This segment should explain the naming convention for higher levels of hierarchy: https://github.com/bids-standard/bids-specification/blob/master/src/02-common-principles.md#the-inheritance-principle Please let me know if something is not clear.

@VisLab
Copy link
Member Author

VisLab commented Oct 8, 2018

Yes that is clear. However, can you have something above subj01.... that is the global HED files usually apply to all subjects. Can you do it above subject? Otherwise if you have 20 subjects in your experiment, you have to repeat 20 times.

Also, I corrected the typo, but pressed the commit before writing the explanatory comment. Sorry, I'll be more careful next time....

@chrisgorgo
Copy link
Contributor

You can have task-nback_events.json in the root of your dataset and it will apply to all events files for task nback across participants and sessions. This is where global HED tags could be specified.

@VisLab
Copy link
Member Author

VisLab commented Oct 8, 2018 via email

@chrisgorgo
Copy link
Contributor

Great - this is shaping very nicely.

Regarding your comment about unhappy analysts: I don't understand why you would not be able to do look for time-locked responses to events with a specific value of trial_type (or a custom column to be honest). It is basically categorization of events and whether a time-locked analysis can be done should depend on the design of the experiment.

It seems that you have a specific definition of event_type that is different from trial_type. Would be good to know what is it precisely and how does it differ from trial_type.

@VisLab
Copy link
Member Author

VisLab commented Oct 8, 2018 via email

@nicholst
Copy link
Collaborator

nicholst commented Oct 9, 2018 via email

@sappelhoff
Copy link
Member

With the standard as it stands --- I would have no idea what to do with the data.

With BIDS EEG we are introducing a new column called event_value. This marks a TTL marker value to an event. Then, there will be a json object in the corresponding _events.json which will map the TTL marker values to their meaning. Please read the BEP 006, which is very relevant to the HED discussion as well --> https://docs.google.com/document/d/1ArMZ9Y_quTKXC-jNXZksnedK2VHHoKP3HCeO5HPcgLE

Example for the events.tsv

onset    duration    trial_type    event_value
1.1    n/a    left    1
1.3    n/a    left    2
1.6    n/a    left    3
2.0    n/a    left    4
...

Example for the corresponding events.json

{
    "trial_type": {
        "LongName": "trial type",
        "Description": "Which hand was lifted by the participant.",
        "Levels": {
            "left": "Left hand was lifted",
            "right": "Right hand was lifted"
        }
    },
    "event_value": {
        "LongName": "event value",
        "Description": "Value of a TTL trigger that was sent to mark the event",
        "Levels": {
            "1": "onset of fixation cross",
            "2": "onset of stimulus",
            "3": "response of participant",
            "4": "feedback displayed",
        }
    }
}

I think that all the "infrastructure" we need for HED tagging the events is already present but might be a bit scattered across BEPs and the spec / or not adequately documented. This is also why this PR is important.

@VisLab
Copy link
Member Author

VisLab commented Oct 9, 2018

Stefan's example clears things up --- however, I think there is confusion about what trial_type can be. I read the EEG proposed extension and am basically happy with it, though there are several points that are not perfectly clear.

Does the BIDS standard allow the researcher to include as many additional named columns as they want and can those named columns can have optional maps to HED tags in the _events.json? This would give the most flexibility. Where should this be clarified? Should there be examples? When the EEG bids extension is adapted, will the event_value column be included in BIDS as a whole, or only when it pertains to EEG studies.

@chrisgorgo
Copy link
Contributor

Does the BIDS standard allow the researcher to include as many additional named columns as they want?

Yes. As per https://github.com/bids-standard/bids-specification/blob/master/src/04-modality-specific-files/03-task-events.md "An arbitrary number of additional columns can be added."

Can those named columns can have optional maps to HED tags in the _events.json? This would give the most flexibility. Where should this be clarified? Should there be examples?

This is currently unclear, but I don't see why not. This could be clarified in this PR in the HED appendix. Examples will be useful.

When the EEG bids extension is adapted, will the event_value column be included in BIDS as a whole, or only when it pertains to EEG studies.

This is a bit of a separate discussion - I don't know what is the current scope of the EEG proposal.

@chrisgorgo
Copy link
Contributor

@VisLab this seems to be close to ready to merge. Only a couple of clarifications and rebase to resolve conflicts left. Are there any outstanding questions we can help with?

@VisLab
Copy link
Member Author

VisLab commented Oct 25, 2018 via email

@chrisgorgo
Copy link
Contributor

chrisgorgo commented Oct 25, 2018 via email

@VisLab
Update 03_hed.md
45c2712 
This needs an additional going over.
@VisLab
Update 03_hed.md
9cb7c84 
Could you all review and merge if acceptable.
@VisLab
Update 03_hed.md
e8da080 
Corrected a couple of typos.
@VisLab
Copy link
Member Author

VisLab commented Oct 27, 2018 via email

@chrisgorgo
Copy link
Contributor

I still see conflicts - could you rebase or merge master?

@chrisgorgo
Copy link
Contributor

After getting an updated master from upstream (https://github.com/bids-standard/bids-specification) that is.

sappelhoff
sappelhoff reviewed Oct 27, 2018
View reviewed changes
src/99_appendices/03_hed.md
Since dedicated fields already exist for the overall task classification (`CogAtlasID` and `CogPOID`) in the sidecar JSON files HED
tags from the Paradigm subcategory should not be used to annotate
events.
HED is a controlled vocabulary of terms describing events in a behavioural paradigm. HED was originally developed with EEG in mind, but is applicable to all behavioural experiments. Each level of a hierarchical tag is delimited with a forward slash ("/"). A HED string contains one or more HED tags separated by commas. Parentheses (brackets) group tags and enable specification of multiple items and their attributes in a single HED string (see section 2.4 in [HED Tagging Strategy Guide](http://www.hedtags.org/downloads/HED%20Tagging%20Strategy%20Guide.pdf)). For more information about HED and tools available to validate and match HED strings, please visit [www.hedtags.org](http://www.hedtags.org). Since dedicated fields already exist for the overall task classification (`CogAtlasID` and `CogPOID`) in the sidecar JSON files HED tags from the Paradigm subcategory should not be used to annotate events.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
HED is a controlled vocabulary of terms describing events in a behavioural paradigm. HED was originally developed with EEG in mind, but is applicable to all behavioural experiments. Each level of a hierarchical tag is delimited with a forward slash ("/"). A HED string contains one or more HED tags separated by commas. Parentheses (brackets) group tags and enable specification of multiple items and their attributes in a single HED string (see section 2.4 in [HED Tagging Strategy Guide](http://www.hedtags.org/downloads/HED%20Tagging%20Strategy%20Guide.pdf)). For more information about HED and tools available to validate and match HED strings, please visit [www.hedtags.org](http://www.hedtags.org). Since dedicated fields already exist for the overall task classification (`CogAtlasID` and `CogPOID`) in the sidecar JSON files HED tags from the Paradigm subcategory should not be used to annotate events.
HED is a controlled vocabulary of terms describing events in a behavioural paradigm. HED was originally developed with EEG in mind, but is applicable to all behavioural experiments. Each level of a hierarchical tag is delimited with a forward slash ("/"). A HED string contains one or more HED tags separated by commas. Parentheses (brackets) group tags and enable specification of multiple items and their attributes in a single HED string (see section 2.4 in [HED Tagging Strategy Guide](http://www.hedtags.org/downloads/HED%20Tagging%20Strategy%20Guide.pdf)). For more information about HED and tools available to validate and match HED strings, please visit [www.hedtags.org](http://www.hedtags.org). Since dedicated fields already exist for the overall task classification (`CogAtlasID` and `CogPOID`) in the sidecar JSON files, HED tags from the Paradigm subcategory should not be used to annotate events.

Copy link
Member

Choose a reason for hiding this comment

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

note, I just added a comma between

... in the sidecar JSON files

and

... HED tags from the

sappelhoff
sappelhoff reviewed Oct 27, 2018
View reviewed changes
src/99_appendices/03_hed.md
1.2 0.6 fixationCross Event/Category/Experimental stimulus, Event/Label/CrossFix, Event/Description/A cross appears at screen center to serve as a fixation point, Sensory presentation/Visual, Item/Object/2D Shape/Cross, Attribute/Visual/Fixation point, Attribute/Visual/Rendering type/Screen, Attribute/Location/Screen/Center
5.6 0.008 target Event/Label/Target image, Event/Description/A white airplane as the RSVP target superimposed on a satellite image is displayed., Event/Category/Experimental stimulus, (Item/Object/Vehicle/Aircraft/Airplane, Participant/Effect/Cognitive/Target, Sensory presentation/Visual/Rendering type/Screen/2D), (Item/Natural scene/Arial/Satellite, Sensory presentation/Visual/Rendering type/Screen/2D)
500 0.008 nontarget Event/Label/Non-target image, Event/Description/A non-target image is displayed for about 8 milliseconds, Event/Category/Experimental stimulus, (Item/Natural scene/Arial/Satellite, Participant/Effect/Cognitive/Expected/Non-target, Sensory presentation/Visual/Rendering type/Screen/2D), Attribute/Onset
onset duration mycodes HED
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onset duration mycodes HED
onset duration mycodes

Copy link
Member

Choose a reason for hiding this comment

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

you are not using the HED column here, so I'd remove it

sappelhoff
sappelhoff approved these changes Oct 27, 2018
View reviewed changes
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Looks good to me as well (after pulling from master and rebasing). I made two minor suggestions, feel free to use them if you also think they make sense.

Other than that it's really clear now, thanks @VisLab!

@VisLab
Copy link
Member Author

VisLab commented Oct 27, 2018

These suggestions look good to me. It is good to go. I would appreciate someone else doing the actual merge. Thanks.

@chrisgorgo
Copy link
Contributor

Thank you @VisLab.

I am pretty busy right now - @sappelhoff would you mind helping out and opening a new PR based on @VisLab branch but synced with master and with conflicts resolved. It would be a great help.

@sappelhoff sappelhoff mentioned this pull request Oct 27, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisgorgo chrisgorgo chrisgorgo left review comments

@sappelhoff sappelhoff sappelhoff approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VisLab @chrisgorgo @sappelhoff @nicholst