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

[ENH] add screen parameters metadata #1369

Merged
merged 6 commits into from
Dec 24, 2022

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Dec 9, 2022

Extracts screen parameters metadata from BEP20 and adds them to events.tsv associated metadata to make them usable by other modalities.


EDIT

Clarification: those extra metadata are meant to go in the events.json


Motivation:

The metadata listed below are not eyetracking specific and can in principle be used by any experiment that uses visual stimulation. Those metadata are actually fairly important for anyone who would like to replicate an experiment where visual stimulus relative size matters.

  • ScreenDistance
  • ScreenRefreshRate
  • ScreenResolution
  • ScreenSize

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Dec 9, 2022

@mszinte as discussed I am extracting this part of BEP020.

@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Base: 88.65% // Head: 88.65% // No change to project coverage 👍

Coverage data is based on head (a310c56) compared to base (2b69916).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1369   +/-   ##
=======================================
  Coverage   88.65%   88.65%           
=======================================
  Files          11       11           
  Lines        1084     1084           
=======================================
  Hits          961      961           
  Misses        123      123           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Dec 9, 2022

@effigies

The requirement level for those metadata is currently recommended.

For eyetracking some will have to be required, so this may require overriding the StimulusPresentation subobject for eyetrcaking to be more like this.

StimulusPresentation:
  name: StimulusPresentation
  display_name: Stimulus Presentation
  description: |
    Object containing key-value pairs related to the software used to present
    the stimuli during the experiment, specifically:
    `"OperatingSystem"`, `"SoftwareName"`, `"SoftwareRRID"`, `"SoftwareVersion"` and
    `"Code"`.
    See table below for more information.
  type: object
  recommended_fields:
    - OperatingSystem
    - ScreenRefreshRate
    - SoftwareName
    - SoftwareRRID
    - SoftwareVersion
    - Code
  required_fields: #   <---------  those fields are required
    - ScreenDistance
    - ScreenResolution
    - ScreenSize
  properties:
    OperatingSystem:
      $ref: objects.metadata.OperatingSystem
    ScreenDistance:
      $ref: objects.metadata.ScreenDistance
    ScreenRefreshRate:
      $ref: objects.metadata.ScreenRefreshRate
    ScreenResolution:
      $ref: objects.metadata.ScreenResolution
    ScreenSize:
      $ref: objects.metadata.ScreenSize
    SoftwareName:
      $ref: objects.metadata.SoftwareName
    SoftwareRRID:
      $ref: objects.metadata.SoftwareRRID
    SoftwareVersion:
      $ref: objects.metadata.SoftwareVersion
    Code:
      $ref: objects.metadata.Code

Let me know if you anticipate any headaches with this

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.

+1 to include this in the experiment metadata

src/schema/objects/metadata.yaml Outdated Show resolved Hide resolved
src/schema/objects/metadata.yaml Outdated Show resolved Hide resolved
@effigies
Copy link
Collaborator

effigies commented Dec 9, 2022

I think we could do either an override or just a separate check for those values.

@effigies
Copy link
Collaborator

effigies commented Dec 9, 2022

The current description is

Object containing key-value pairs related to the software used to present
the stimuli during the experiment, specifically:
"OperatingSystem", "SoftwareName", "SoftwareRRID", "SoftwareVersion" and
"Code".

If you want to make this include hardware and positioning details, then the description will need to be udpated. Alternately you could create a new metadata field like PresentationHardware.

@CPernet
Copy link
Collaborator

CPernet commented Dec 9, 2022

I don't understand why/how this would go into events.tsv? I do get your point that this is a generic type of info.
The reason why it doesn't make sense to me to put that in events.tsv is that for 99% of use cases, the screen distance, refresh rate, resolution and size doesn't change - which means replicating the same values over and over again in the tsv file.
Is is possible to store the info in the events.json even if the key/values do not correspond to columns of the tsv?

@robertoostenveld
Copy link
Collaborator

@CPernet I think that the events.json is meant by the "metadata for the events.tsv". That would place it along https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/05-task-events.html#stimulus-presentation-details

@CPernet
Copy link
Collaborator

CPernet commented Dec 10, 2022

That's what I was getting at ... Just want to avoid the repetition of values in the tsv. Where else could that go? Or do we want a full optional object StimulusPresentation

@Remi-Gau
Copy link
Collaborator Author

I don't understand why/how this would go into events.tsv? I do get your point that this is a generic type of info. The reason why it doesn't make sense to me to put that in events.tsv is that for 99% of use cases, the screen distance, refresh rate, resolution and size doesn't change - which means replicating the same values over and over again in the tsv file. Is is possible to store the info in the events.json even if the key/values do not correspond to columns of the tsv?

yup as mentioned by @robertoostenveld I meant the events.json. Will add a correction in the top message. Sorry for the confusion.

Remi-Gau and others added 2 commits December 10, 2022 11:31
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
@CPernet
Copy link
Collaborator

CPernet commented Dec 10, 2022

I don't think that's what @robertoostenveld meant -- I guess the verification here is 1 - all columns of the tsv should be described in json but 2 - can json have more elements than tsv

@Remi-Gau
Copy link
Collaborator Author

If you want to make this include hardware and positioning details, then the description will need to be udpated. Alternately you could create a new metadata field like PresentationHardware.

OK I decided to actually remove from the description the list of fields that this object should include because it felt redundant to list them verbatim and then have a table rendered to list them again.

@Remi-Gau
Copy link
Collaborator Author

2 - can json have more elements than tsv

Yes they can.

Whether to describe the stimuli database used or some of the presentation details:

{
    "trial_type": {
        "LongName":   "Emotion image type",
        "Description": "Type of emotional face from Karolinska database that is displayed",
        "Levels": {
            "afraid": "A face showing fear is displayed",
            "angry":  "A face showing anger is displayed",
            "sad":    "A face showing sadness is displayed"
        }
    },
    "identifier": {
        "LongName": "Unique identifier from Karolinska (KDEF) database",
        "Description": "ID from KDEF database used to identify the displayed image"
    },
    "StimulusPresentation": {
        "OperatingSystem": "Linux Ubuntu 18.04.5",
        "SoftwareName": "Psychtoolbox",
        "SoftwareRRID": "SCR_002881",
        "SoftwareVersion": "3.0.14",
        "Code": "doi:10.5281/zenodo.3361717"
    }
}

@CPernet
Copy link
Collaborator

CPernet commented Dec 10, 2022

ok good (because of course in tsv that was weird)

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@yarikoptic
Copy link
Collaborator

BTW thank you for "extracting" this as a separate PR. A great example for #371

@Remi-Gau Remi-Gau merged commit 83e20a1 into bids-standard:master Dec 24, 2022
@Remi-Gau Remi-Gau deleted the screen_param branch December 24, 2022 08:14
@Remi-Gau
Copy link
Collaborator Author

Been more than 5 days.

Merging.

@@ -268,7 +268,11 @@ in the accompanying JSON sidecar as follows (based on the example of the previou
"SoftwareName": "Psychtoolbox",
"SoftwareRRID": "SCR_002881",
"SoftwareVersion": "3.0.14",
"Code": "doi:10.5281/zenodo.3361717"
"Code": "doi:10.5281/zenodo.3361717",
"ScreenDistance": 180,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Remi-Gau - even though unit was adjusted to become m, examples seems were not like here and below!... I will send a PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops. Thanks for spotting it.

yarikoptic added a commit to yarikoptic/bids-specification that referenced this pull request Dec 25, 2022
sappelhoff pushed a commit that referenced this pull request Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants