-
Notifications
You must be signed in to change notification settings - Fork 170
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
[FIX] Specify marker filenames for KIT data (MEG) #62
[FIX] Specify marker filenames for KIT data (MEG) #62
Conversation
Looks reasonable to me |
Thank you for opening the PR. I got one concern: you are using the |
I just did this because it's what @robertoostenveld and @teonbrooks recommended.
I don't think this has to be an entirely new key-value pair that can be used elsewhere in the specification. This seems to be a unique scenario so I don't see why we can't just have something unique for it. |
Any thoughts @chrisfilo @teonbrooks ? Would be nice to get this sorted. |
I would prefer new tag (like |
since |
What would be the suffix if the key-value pair is
? |
This is a good question. There should be a prefix. |
Could just reuse |
I think the most backwards compatible solution would be to leave the suffix as it is ( |
I would like a solution that scales to other types of data and not only KIT. Repeated measurements of 3-D coordinates of specific points (landmarks, fiducials=markers=headcoils, electrodes) with a digitizer (like a Polhemus) also can happens for CTF MEG recordings, but also EEG recordings, and might also happen in iEEG datasets. The issue reminds me of a discussion that we had with "events" where it is considered desirable to have post-hoc "annotations" (i.e. events that are based on visual inspection of the data, i.e. offline) separate from the deterministic/objective events represented in the events.tsv file. Here you could think of spike or seizure detection, possibly done with different methods. The format would always be the same as for the events.tsv file, but multiple files are desired. That is also what we have for the marker files: there can be one or multiple (calling them pre/post would limit it to 2 files). Just from a more principled viewpoint: A good (generic/scalable) solution would use a key-value pair. As an analogy: I suppose that run-1, run-2, ... would be used for repeated anatomical MRIs with the exactly the same scan parameters. The last part of the file should describe the type of data contained in the file (headshape, meg, eeg) and the extension the file format (ds, sqd, fif). |
so I was looking through the specification for something else and noticed that the |
I think |
would it matter if were used for a different context for a pretty niche case? |
I would prefer to avoid such internal inconsistency by picking a different keyword. |
@chrisfilo what would you suggest that maintains internal consistency? I suppose trying to use any of the existing keys would lead to consistency issues, so adding a new key-value pair seems to be the only solution. |
Yes. I think adding a new keyword instead of redefining an old one would improve the consistency and make the standard more intuitive for people working with multiple modalities. |
I completely see the justification for wanting to introduce another kvp to avoid inconsistencies, however it just seems silly to add another for just one single case. Finally do you have a suggestion for an appropriate key? I can't really think of one that would be good. If we don't care about having the kvp used elsewhere then we could just use |
I propose to consider this also in relation to EEG and especially iEEG specifications. For the iEEG it is not uncommon to have pre and post-operative data. For EEG it would be possible (but not common) to have pre- and post-measurement electrode digitizations. |
besides the two |
maybe having |
Any further thoughts on this? It would be quite nice to get this sorted as it blocks a few things that I am working on and the sooner it is sorted the less impact it has on all the data converted to BIDS format by where I work. |
The suffix is already specified as headshape, i.e. as In the thread above Re: restricted versus free form values: I think that just |
I share both @monkeyman192 and @robertoostenveld's opinion about not having too many key-value pairs with overlapping (or even identical / synonymous) meaning. This adds to the confusion of the user rather than ease it. Supporting what @robertoostenveld said furthermore about unrestricted values for the new key value pair (kvp), I would suggest that for MEG I am in favor of re-using the Excerpt from the spec
Example
|
I agree with the views shared above by @robertoostenveld and @sappelhoff . We should reuse the |
657647a
to
903b90a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to please incorporate #19 (comment) in this PR
c625734
to
978960e
Compare
@sappelhoff ok, rebased and hopefully addressed your comment as well as addressing @yarikoptic 's comment and @robertoostenveld 's comment (I did reword it a bit however as I think the wording wasn't quite right. Hopefully you agree!!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @monkeyman192!
I just tested adding an acq
parameter to a markers file by temporarily modifying the type.spec.js
file in my local validator version and it works. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @monkeyman192. I also opened an issue on the validator that the order of parameters is not checked (at least for run
and acq
).
@jasmainak can you review and help Matt to get this merged? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The table is a bit hard to read. It would be nice if the CIs also build the doc and show it in the artifacts tab
|
Oh nice, sorry missed it. Thanks! And PR looks good to me. |
thanks @monkeyman192 |
Text is using both 'file name' and 'filename' pretty much to the equal amount ATM (see git grep outputs below). Code uses 'filename', and wikipedia has https://en.wikipedia.org/wiki/Filename and prefers to use 'filename' in it. So I decided to harmonize into 'filename'. $> git grep 'file name' | grep '\.md' | grep -v MACRO | nl 1 src/02-common-principles.md:1. **File extension** - a portion of the file name after the left-most 2 src/02-common-principles.md:are compulsory. For example a particular file name format is required when 3 src/02-common-principles.md:saved under a particular file name specified in the standard. This standard 4 src/02-common-principles.md:A file name consists of a chain of *entities*, or key-value pairs, a *suffix* and an 5 src/02-common-principles.md:`subject`, the file name MUST begin with the string `sub-<label>_ses-<label>`. 6 src/02-common-principles.md:If the `session` level is omitted in the folder structure, the file name MUST begin 7 src/02-common-principles.md:key/value pair MUST also be included as part of the file names themselves. 8 src/02-common-principles.md:produces a human readable file name, such as `sub-01_task-rest_eeg.edf`. 9 src/02-common-principles.md:It is evident from the file name alone that the file contains resting state 10 src/02-common-principles.md:Entities within a file name MUST be unique. 11 src/02-common-principles.md:For example, the following file name is not valid because it uses the `acq` 12 src/02-common-principles.md:label, but must be included in file names (similarly to other key names). 13 src/02-common-principles.md:meaning of file names and setting requirements on their contents or metadata. 14 src/02-common-principles.md:to suppress warnings or provide interpretations of your file names. 15 src/03-modality-agnostic-files.md:This file is REQUIRED if `sample-<label>` is present in any file name within the dataset. 16 src/04-modality-specific-files/05-task-events.md:Where `<matches>` corresponds to task file name. For example: 17 src/04-modality-specific-files/06-physiological-and-other-continuous-recordings.md:In the template file names, the `<matches>` part corresponds to task file name 18 src/05-derivatives/02-common-data-types.md:is used to prevent clashing with the original file name. 19 src/06-longitudinal-and-multi-site-studies.md:and [file names](02-common-principles.md#file-name-structure) 20 src/99-appendices/03-hed.md:screen or the file name of the stimulus image. 21 src/99-appendices/03-hed.md: "LongName": "Stimulus file name", 22 src/99-appendices/04-entity-table.md:[file name structure](../02-common-principles.md#file-name-structure), 23 src/99-appendices/06-meg-file-formats.md:that not only the file names, but also the internal file pointers will be 24 src/99-appendices/09-entities.md:[file name structure](../02-common-principles.md#file-name-structure). 25 src/CHANGES.md:- \[FIX] Clarify use of session entity in file names [bids-standard#532](bids-standard#532) ([Moo-Marc](https://github.com/Moo-Marc)) 26 src/CHANGES.md:- \[FIX] Specify marker file names for KIT data (MEG) [bids-standard#62](bids-standard#62) ([monkeyman192](https://github.com/monkeyman192)) 27 src/CHANGES.md:- Added missing `sub-<participant_label>` in behavioral data file names. 28 src/pregh-changes.md:- Added missing `sub-<participant_label>` in behavioral data file names. $> git grep 'filename' | grep '\.md' | grep -v MACRO | nl 1 CONTRIBUTING.md:Make sure that all filename format templates, entity tables, and entity definitions are correct 2 src/02-common-principles.md:(with the same filename as the `.nii[.gz]` file, but with a `.json` extension). 3 src/03-modality-agnostic-files.md: "filename": ("REQUIRED", "There MUST be exactly one row for each file."), 4 src/03-modality-agnostic-files.md:filename acq_time 5 src/04-modality-specific-files/02-magnetoencephalography.md:which saves the MEG sensor coil positions in a separate file with two possible filename extensions (`.sqd`, `.mrk`). 6 src/05-derivatives/01-introduction.md: status. Any modification of raw files must use a modified filename that does 7 src/05-derivatives/01-introduction.md: not conflict with the raw filename. Further, any files created as part of a 8 src/05-derivatives/01-introduction.md: derivative dataset must not match a permissible filename of a valid raw 9 src/05-derivatives/01-introduction.md: dataset. Stated equivalently, if any filename in a derivative dataset has a 10 src/05-derivatives/01-introduction.md:- Each Derivatives filename MUST be of the form: 11 src/05-derivatives/01-introduction.md: `source_entities` MUST be the entire source filename, with the omission of 12 src/05-derivatives/01-introduction.md: the source suffix and extension. One exception to this rule is filename 13 src/05-derivatives/01-introduction.md:- There is no prohibition against identical filenames in different derived 14 src/05-derivatives/03-imaging.md:filename. 15 src/99-appendices/04-entity-table.md:specification, and establishes a common order within a filename. 16 src/99-appendices/08-coordinate-systems.md:The `scanner` coordinate system is implicit and assumed by default if the derivative filename does not define **any** `space-<label>`. 17 src/99-appendices/11-qmri.md:filenames will remain the same; however, the optional metadata (third column) may 18 src/CHANGES.md:- \[SCHEMA] Use macro for filename templates in file collections appendix [bids-standard#787](bids-standard#787) ([tsalo](https://github.com/tsalo)) 19 src/CHANGES.md:- \[FIX] Accidentally swapped Neuromag/Elekta/MEGIN cross-talk & fine-calibration filename extensions [bids-standard#621](bids-standard#621) ([hoechenberger](https://github.com/hoechenberger)) 20 src/CHANGES.md:- \[INFRA] SCHEMA: Declare entities by concept names, add entity field for filename components [bids-standard#616](bids-standard#616) ([effigies](https://github.com/effigies)) 21 src/CHANGES.md:- \[FIX] Common principles: Fix filename in inheritance principle [bids-standard#261](bids-standard#261) ([Lestropie](https://github.com/Lestropie)) 22 src/CHANGES.md:- \[FIX] Example for IntendedFor was missing session indicator in the filename [bids-standard#129](bids-standard#129) ([yarikoptic](https://github.com/yarikoptic)) 23 src/schema/README.md:the entity tables, entity definitions, filename templates, and metadata tables. 24 src/schema/README.md:- `entities.yaml`: Entities (key/value pairs in folder and filenames). 25 src/schema/README.md:This file contains a dictionary in which each entity (key/value pair in filenames) is defined. 26 src/schema/README.md:they appear in filenames _and_ their full names. 27 src/schema/README.md:For example, the key for the "Contrast Enhancing Agent" entity, which appears in filenames as `ce-<label>`, 28 src/schema/README.md:since many entities (such as `ce`) have very short filename elements. 29 src/schema/README.md:The `entity` field is the entity as it appears in filenames. For example, the `entity` for `ceagent` is `ce`. 30 src/schema/README.md:Given that all entities appear in filenames, they should all be strings and the `type` field should always be `string`. 31 src/schema/README.md:For example, `run` should have an index, so a valid key-value pair in a filename would be `run-01`. 32 src/schema/README.md:Keys are the filenames (without file extensions), 33 src/schema/README.md:- `datatypes/*.yaml`: Files in the `datatypes` folder contain information about valid filenames within a given datatype. 34 src/schema/README.md: Each dictionary contains a list of suffixes, entities, and file extensions which may constitute a valid BIDS filename. 35 src/schema/README.md:- `entities.yaml`: This file simply defines the order in which entities, when present, MUST appear in filenames. 36 src/schema/README.md:Each dictionary corresponds to a group of suffixes that have the same rules regarding filenames. 37 src/schema/README.md:**NOTE**: The order in which entities appear in these dictionaries does not reflect how they should appear in filenames. 38 src/schema/README.md:This file contains a list of entities in the order in which they must appear in filenames. === Do not change lines below === { "chain": [], "cmd": "bash -c 'git grep -l '\"'\"'file name'\"'\"' | grep '\"'\"'\\.md'\"'\"' | grep -v MACRO | xargs sed -i -e '\"'\"'s,file name,filename,g'\"'\"''", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^
Text is using both 'file name' and 'filename' pretty much to the equal amount ATM (see git grep outputs below). Code uses 'filename', and wikipedia has https://en.wikipedia.org/wiki/Filename and prefers to use 'filename' in it. So I decided to harmonize into 'filename'. $> git grep 'file name' | grep '\.md' | grep -v MACRO | nl 1 src/02-common-principles.md:1. **File extension** - a portion of the file name after the left-most 2 src/02-common-principles.md:are compulsory. For example a particular file name format is required when 3 src/02-common-principles.md:saved under a particular file name specified in the standard. This standard 4 src/02-common-principles.md:A file name consists of a chain of *entities*, or key-value pairs, a *suffix* and an 5 src/02-common-principles.md:`subject`, the file name MUST begin with the string `sub-<label>_ses-<label>`. 6 src/02-common-principles.md:If the `session` level is omitted in the folder structure, the file name MUST begin 7 src/02-common-principles.md:key/value pair MUST also be included as part of the file names themselves. 8 src/02-common-principles.md:produces a human readable file name, such as `sub-01_task-rest_eeg.edf`. 9 src/02-common-principles.md:It is evident from the file name alone that the file contains resting state 10 src/02-common-principles.md:Entities within a file name MUST be unique. 11 src/02-common-principles.md:For example, the following file name is not valid because it uses the `acq` 12 src/02-common-principles.md:label, but must be included in file names (similarly to other key names). 13 src/02-common-principles.md:meaning of file names and setting requirements on their contents or metadata. 14 src/02-common-principles.md:to suppress warnings or provide interpretations of your file names. 15 src/03-modality-agnostic-files.md:This file is REQUIRED if `sample-<label>` is present in any file name within the dataset. 16 src/04-modality-specific-files/05-task-events.md:Where `<matches>` corresponds to task file name. For example: 17 src/04-modality-specific-files/06-physiological-and-other-continuous-recordings.md:In the template file names, the `<matches>` part corresponds to task file name 18 src/05-derivatives/02-common-data-types.md:is used to prevent clashing with the original file name. 19 src/06-longitudinal-and-multi-site-studies.md:and [file names](02-common-principles.md#file-name-structure) 20 src/99-appendices/03-hed.md:screen or the file name of the stimulus image. 21 src/99-appendices/03-hed.md: "LongName": "Stimulus file name", 22 src/99-appendices/04-entity-table.md:[file name structure](../02-common-principles.md#file-name-structure), 23 src/99-appendices/06-meg-file-formats.md:that not only the file names, but also the internal file pointers will be 24 src/99-appendices/09-entities.md:[file name structure](../02-common-principles.md#file-name-structure). 25 src/CHANGES.md:- \[FIX] Clarify use of session entity in file names [#532](#532) ([Moo-Marc](https://github.com/Moo-Marc)) 26 src/CHANGES.md:- \[FIX] Specify marker file names for KIT data (MEG) [#62](#62) ([monkeyman192](https://github.com/monkeyman192)) 27 src/CHANGES.md:- Added missing `sub-<participant_label>` in behavioral data file names. 28 src/pregh-changes.md:- Added missing `sub-<participant_label>` in behavioral data file names. $> git grep 'filename' | grep '\.md' | grep -v MACRO | nl 1 CONTRIBUTING.md:Make sure that all filename format templates, entity tables, and entity definitions are correct 2 src/02-common-principles.md:(with the same filename as the `.nii[.gz]` file, but with a `.json` extension). 3 src/03-modality-agnostic-files.md: "filename": ("REQUIRED", "There MUST be exactly one row for each file."), 4 src/03-modality-agnostic-files.md:filename acq_time 5 src/04-modality-specific-files/02-magnetoencephalography.md:which saves the MEG sensor coil positions in a separate file with two possible filename extensions (`.sqd`, `.mrk`). 6 src/05-derivatives/01-introduction.md: status. Any modification of raw files must use a modified filename that does 7 src/05-derivatives/01-introduction.md: not conflict with the raw filename. Further, any files created as part of a 8 src/05-derivatives/01-introduction.md: derivative dataset must not match a permissible filename of a valid raw 9 src/05-derivatives/01-introduction.md: dataset. Stated equivalently, if any filename in a derivative dataset has a 10 src/05-derivatives/01-introduction.md:- Each Derivatives filename MUST be of the form: 11 src/05-derivatives/01-introduction.md: `source_entities` MUST be the entire source filename, with the omission of 12 src/05-derivatives/01-introduction.md: the source suffix and extension. One exception to this rule is filename 13 src/05-derivatives/01-introduction.md:- There is no prohibition against identical filenames in different derived 14 src/05-derivatives/03-imaging.md:filename. 15 src/99-appendices/04-entity-table.md:specification, and establishes a common order within a filename. 16 src/99-appendices/08-coordinate-systems.md:The `scanner` coordinate system is implicit and assumed by default if the derivative filename does not define **any** `space-<label>`. 17 src/99-appendices/11-qmri.md:filenames will remain the same; however, the optional metadata (third column) may 18 src/CHANGES.md:- \[SCHEMA] Use macro for filename templates in file collections appendix [#787](#787) ([tsalo](https://github.com/tsalo)) 19 src/CHANGES.md:- \[FIX] Accidentally swapped Neuromag/Elekta/MEGIN cross-talk & fine-calibration filename extensions [#621](#621) ([hoechenberger](https://github.com/hoechenberger)) 20 src/CHANGES.md:- \[INFRA] SCHEMA: Declare entities by concept names, add entity field for filename components [#616](#616) ([effigies](https://github.com/effigies)) 21 src/CHANGES.md:- \[FIX] Common principles: Fix filename in inheritance principle [#261](#261) ([Lestropie](https://github.com/Lestropie)) 22 src/CHANGES.md:- \[FIX] Example for IntendedFor was missing session indicator in the filename [#129](#129) ([yarikoptic](https://github.com/yarikoptic)) 23 src/schema/README.md:the entity tables, entity definitions, filename templates, and metadata tables. 24 src/schema/README.md:- `entities.yaml`: Entities (key/value pairs in folder and filenames). 25 src/schema/README.md:This file contains a dictionary in which each entity (key/value pair in filenames) is defined. 26 src/schema/README.md:they appear in filenames _and_ their full names. 27 src/schema/README.md:For example, the key for the "Contrast Enhancing Agent" entity, which appears in filenames as `ce-<label>`, 28 src/schema/README.md:since many entities (such as `ce`) have very short filename elements. 29 src/schema/README.md:The `entity` field is the entity as it appears in filenames. For example, the `entity` for `ceagent` is `ce`. 30 src/schema/README.md:Given that all entities appear in filenames, they should all be strings and the `type` field should always be `string`. 31 src/schema/README.md:For example, `run` should have an index, so a valid key-value pair in a filename would be `run-01`. 32 src/schema/README.md:Keys are the filenames (without file extensions), 33 src/schema/README.md:- `datatypes/*.yaml`: Files in the `datatypes` folder contain information about valid filenames within a given datatype. 34 src/schema/README.md: Each dictionary contains a list of suffixes, entities, and file extensions which may constitute a valid BIDS filename. 35 src/schema/README.md:- `entities.yaml`: This file simply defines the order in which entities, when present, MUST appear in filenames. 36 src/schema/README.md:Each dictionary corresponds to a group of suffixes that have the same rules regarding filenames. 37 src/schema/README.md:**NOTE**: The order in which entities appear in these dictionaries does not reflect how they should appear in filenames. 38 src/schema/README.md:This file contains a list of entities in the order in which they must appear in filenames. === Do not change lines below === { "chain": [], "cmd": "bash -c 'git grep -l '\"'\"'file name'\"'\"' | grep '\"'\"'\\.md'\"'\"' | grep -v MACRO | xargs sed -i -e '\"'\"'s,file name,filename,g'\"'\"''", "exit": 0, "extra_inputs": [], "inputs": [], "outputs": [], "pwd": "." } ^^^ Do not change lines above ^^^
closes #45