BEP029 Motion (examples) by sappelhoff · Pull Request #348 · bids-standard/bids-examples

sappelhoff · 2023-02-02T16:30:24Z

This PR is to be merged into the master branch when the BEP029 Motion is integrated into the BIDS specification.

You can properly view the three example datasets here:

Leads:

Links:

To do:

revert 7939067
revert changes to ds000248 that somehow creeped into this PR

@sappelhoff

* .json nesting style change, latency channel added * Updated to adhere to spec change, minor corrections. Motion.json contains fields from deprecated coordsys. Other minor differences result from updated version of FT data2bids. * Each tracksys has own json and channels * .json and channels.tsv bug fix, trim events Removed undefined field ExternalSoftwareVersions from motion.json Replaced undfined type values in motion channels.tsv Trimmed events.tsv filed that were too large * motion.json has now doi and funding info Added values for doi and funding fields in motion.json * Undoing mistaken changes to motion.json By mistake I had changed motion.json not dataset_description.json. Now correcting that. * eeg.json wrong fields also removed from body session * [ADD] dataset for dual motion capture systems, no brain imaging * [FIX] remove example dataset from this branch * [ADD] motion systemvalidation dataset * [FIX] smaller comments by @sappelhoff --------- Co-authored-by: sjeung <seinjeung@gmail.com>

to be reverted before merging!

sappelhoff · 2023-02-02T16:37:08Z

@sjeung @JuliusWelzel look at my commit: 7939067

if the motion validator branch is working correctly and the dataset examples here are working as expected, the CI checks for "master" should come back green here. Those for "stable" will remain red.

Before merging this PR, we will revert that commit.

More comments:

next step is to merge [ADD] Motion (BEP029): example data set "dualtask" #329 into the bep029_motion branch
are your examples complete then? I see 3 examples in total then
all examples need to be added to the overall README (see in the diff of this PR), currently I see only one added (motion_systemvalidation, and there is no link to the full data yet)
it seems like some stylistic changes made it into the overall README by accident, can you please remove those again?

sappelhoff · 2023-02-02T16:42:24Z

Update: The CI checks for "master" do not come back green as expected, unfortunately -- could you have a look why that might be the case? Remember to ping Ross in case you need help with the validator branch! :-)

* Add dual task data set * Update motion.json and channels.tsv motion.json has author names in correct format channels.tsv type column has "confidence" renamed to "misc" events are trimmed * Undoing change by mistake and doing the actual fix Authors, Funding, ReferenceAndLinks fields in dataset_description.json are now arrays. Removed wrongly added Authors field in motion.json and eeg.json files. * Update names in channel count fields * Remove headers from motion.tsv * Cut down number of participants For brevity only include 5 participants in the example. * Rename channel count json fields Multi run files were missing in the previous fix * Do not escape slashes * Delete ._.DS_Store * Update .gitignore * Remove invalid component entry * Added dualwalking data set to the main README --------- Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>

JuliusWelzel · 2023-02-03T11:24:13Z

For the systemvalidation dataset I get a code 129 error (1: [ERR] The filename in scans.tsv file does not match what is present in the BIDS dataset. (code: 129 - SCANS_FILENAME_NOT_MATCH_DATASET)). I believe this might be an OS related path issue as the relative path looks fine on my windows machine

* added two motion datasets spotrotation: physical and virtual motion tracking and simultaneous eeg recording dualtask: multiple marker from optical system and simultaneous eeg recording * adjusted names * add stand session * Update participants.tsv * corrected file hierarchy * add example dataset motion_dualsystem_validation Example dataset for the BEP029 was added which includes 2 synchronised data recordings of optical motion capture and inertial measurement units * updated motion dualsystem example dataset * New files added to motion_dualsystem dataset * updated spotrotation and dualtask * updated dataset desc and participant json * empty motion.tsv files and added scans.json * .json nesting style change and latency channels added * Overall update. Deprecated coordsys, single tracksys is presented as array. * [FIX] Updated motion_dualsystem_validation dataset according to new BEP029 dev * Separate json and channel files for different tracksys * Remove other motion data sets than spot-rotation * Channel type fix and json extra field removal * Remove headers, use quaternions * Channel types in .json count fields updated * Cut down number of participants For brevity only include 5 participants in the example. * Remove escaped slashes * Add quat_prefix to quat components And modify wrong entries for components (latency, yaw) * Create README.md * Update README.md * Add motion_spotrotation to main README --------- Co-authored-by: Soeren <65068522+sgrothk@users.noreply.github.com> Co-authored-by: Julius Welzel <julius.welzel@gmail.com> Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>

sappelhoff · 2023-02-03T14:22:35Z

For the systemvalidation dataset I get a code 129 error (1: [ERR] The filename in scans.tsv file does not match what is present in the BIDS dataset. (code: 129 - SCANS_FILENAME_NOT_MATCH_DATASET)). I believe this might be an OS related path issue as the relative path looks fine on my windows machine

right ... I looked at the scans file and it looks good to me. The files that are mentioned there do exist. @rwblair do you have time to look into this?

sappelhoff · 2023-02-08T11:39:43Z

all examples need to be added to the overall README (see in the diff of this PR), currently I see only one added (motion_systemvalidation, and there is no link to the full data yet)

✔️

it seems like some stylistic changes made it into the overall README by accident, can you please remove those again?

fyi, I have done that in b0f825a

sjeung · 2023-02-08T11:53:12Z

all examples need to be added to the overall README (see in the diff of this PR), currently I see only one added (motion_systemvalidation, and there is no link to the full data yet)

✔️

it seems like some stylistic changes made it into the overall README by accident, can you please remove those again?

fyi, I have done that in b0f825a

Thank you Stefan! Now these are all three examples we have at the moment, and only spot rotation is shared fully on OpenNeuro.

sappelhoff

@sjeung @JuliusWelzel there are some things you can do about the examples.

spotrotation seems to pass ✔️
dualtask has some erros
- there are properties in the motion JSON file that are not defined in the JSON schema (note: this is a link to the file on Sein's validator branch, and this is NOT about the BIDS schema; JSON schema is a separate thing only used by the validator currently) --> if the "additionalProperties" key in the JSON schema is false (which it is), then any keys that are present in the motion JSON file but not declared in the JSON schema file will result in an error (Evidence: should NOT have additional properties)
- There is a warning that should be addressed: Tabular file contains custom columns not described in a data dictionary
- And the README for that dataset is missing, which should be remedied
systemvalidation has some problems as well
- one with the scans tsv, mentioned above
- a link to the full data is missing from the overall README
- a dataset specific README is missing

Please let me know if you don't understand what I mean or if you need help looking up the points that I mention.

sappelhoff · 2023-02-08T12:02:52Z

Thank you Stefan! Now these are all three examples we have at the moment, and only spot rotation is shared fully on OpenNeuro.

I see: it's not a requirement that the data are openly shared ... but it'd be preferable. If you share the data in the future, please make sure to add the link here then!

rwblair · 2023-02-08T15:01:41Z

Apologies for how long its taken me to respond.

For the systemvalidation dataset I get a code 129 error (1: [ERR] The filename in scans.tsv file does not match what is present in the BIDS dataset. (code: 129 - SCANS_FILENAME_NOT_MATCH_DATASET)). I believe this might be an OS related path issue as the relative path looks fine on my windows machine

Looks like this is coming from the first line of bidsignore file for that dataset:
https://github.com/bids-standard/bids-examples/blob/bep029_motion/motion_systemvalidation/.bidsignore#L1

I removed the line, but now the files aren't being recognized as bids. First glance the regex looks good. I'll look into this further to see what the issue is.

sappelhoff · 2023-02-08T15:10:14Z

ah nice, thanks Ross. Perhaps we should remove the bidsignore file altogether? Is the remaining **/*mat* needed @JuliusWelzel ?

rwblair · 2023-02-08T15:18:54Z

When testing with the bep_029 validator branch locally the files are recognized and the dataset passes validation.

sappelhoff · 2023-02-08T15:21:44Z

I think in CI it passes as well now. It seems like the only missing thing is for the dualtask dataset what I mentioned above:

there are properties in the motion JSON file that are not defined in the JSON schema (note: this is a link to the file on Sein's validator branch, and this is NOT about the BIDS schema; JSON schema is a separate thing only used by the validator currently) --> if the "additionalProperties" key in the JSON schema is false (which it is), then any keys that are present in the motion JSON file but not declared in the JSON schema file will result in an error (Evidence: should NOT have additional properties)

JuliusWelzel · 2023-02-08T16:27:04Z

ah nice, thanks Ross. Perhaps we should remove the bidsignore file altogether? Is the remaining **/*mat* needed @JuliusWelzel ?

No, I think it can be removed

* Add dual task data set * Update motion.json and channels.tsv motion.json has author names in correct format channels.tsv type column has "confidence" renamed to "misc" events are trimmed * Undoing change by mistake and doing the actual fix Authors, Funding, ReferenceAndLinks fields in dataset_description.json are now arrays. Removed wrongly added Authors field in motion.json and eeg.json files. * Update names in channel count fields * Remove headers from motion.tsv * Cut down number of participants For brevity only include 5 participants in the example. * Rename channel count json fields Multi run files were missing in the previous fix * Do not escape slashes * Delete ._.DS_Store * Update .gitignore * Remove invalid component entry * Added dualwalking data set to the main README * Bidsignore (#351) * Removed patterns in bidsignore which do not correspond to actual files * Added ingored invalid file * Update readme and license * Fix typo in readme * Remove headers from motion.tsv * Update json field name One subject was missing from the last update * Replace invalid compnent entry with n/a Again a subject was missing from last touchup * Compnent column entry for latency channel is n/a * Compnent column entry for latency channel is n/a * Remove undefined fields from json of sub20 --------- Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org> Co-authored-by: Horea Christian <chr@chymera.eu>

* [FIX] reorder columns to match schema * add latest changes * fix dataset name in readme * Remove repeating entries in scans.tsv --------- Co-authored-by: Julius Welzel <julius.welzel@gmail.com>

motion_dualtask/participants.json

...k/sub-07/ses-stand/motion/sub-07_ses-stand_task-dualWalking_tracksys-PhaseSpace1_motion.json

Remi-Gau

Can I be annoying and request that, for one dataset at least, there is a motion.json file with at least some metadata in the root of the dataset as an example of the inheritance principle?

I currently have to update qmri datasets examples + validator to make sure that the inheritance principle is supported for this type of data and I think it is better to test this now rather than fix it after the BEP is merged. :-)

* [FIX] reorder columns to match schema * add latest changes * fix dataset name in readme * Remove repeating entries in scans.tsv * StartTime is removed from spec --------- Co-authored-by: Julius Welzel <julius.welzel@gmail.com>

sjeung · 2023-02-22T09:04:38Z

Can I be annoying and request that for one dataset at least there is a motion.json file with at least some metadata in the root of the dataset as an example of the inheritance principle?

I currently have to update qmri datasets examples + validator to make sure that the inheritance principle is supported for this type of data and I think it is better to test this now rather than fix it after the BEP is merged. :-)

Yeah no problem, @JuliusWelzel I will get to this with the dualtask data set as this would be most beneficial for one like this with lots of files sharing the same metadata

sjeung · 2023-02-22T10:38:38Z

Can I be annoying and request that for one dataset at least there is a motion.json file with at least some metadata in the root of the dataset as an example of the inheritance principle?

I currently have to update qmri datasets examples + validator to make sure that the inheritance principle is supported for this type of data and I think it is better to test this now rather than fix it after the BEP is merged. :-)

Would you have a look at this data set and see if this is implemented the way you meant it?

https://github.com/sjeung/bids-examples/tree/bep029_motion/motion_dualtask

There is now 'task-dualwalking_motion.json' in root directory but now validator throws a new error that there is an invalid field in data directory .json files (not the root one).

Remi-Gau · 2023-02-22T13:22:40Z

Would you have a look at this data set and see if this is implemented the way you meant it?

https://github.com/sjeung/bids-examples/tree/bep029_motion/motion_dualtask

There is now 'task-dualwalking_motion.json' in root directory but now validator throws a new error that there is an invalid field in data directory .json files (not the root one).

that defo looks like what I am looking for.

is the error due to the fact that you have a duplication of TaskDescription?

https://github.com/sjeung/bids-examples/blob/e4e7a3c112d03f77bf59d65676e69ae9d2df1f80/motion_dualtask/task-dualWalking_motion.json#L17

Include age range in "young"

Include age range in "old"

sjeung · 2023-02-24T18:14:56Z

Would you have a look at this data set and see if this is implemented the way you meant it?
https://github.com/sjeung/bids-examples/tree/bep029_motion/motion_dualtask
There is now 'task-dualwalking_motion.json' in root directory but now validator throws a new error that there is an invalid field in data directory .json files (not the root one).

that defo looks like what I am looking for.

is the error due to the fact that you have a duplication of TaskDescription?

https://github.com/sjeung/bids-examples/blob/e4e7a3c112d03f77bf59d65676e69ae9d2df1f80/motion_dualtask/task-dualWalking_motion.json#L17

@Remi-Gau Oh well spotted! I tried removing it, still the same error.

Remi-Gau · 2023-02-24T18:16:12Z

Ok will have a look.

Remi-Gau · 2023-02-25T15:35:38Z

To be sure we are on the same page:

am using your branch of the validator to test those datasets.
running this

bids-validator ~/github/bids/examples/motion_dualtask --config.ignore=99 --config.ignore=36

Getting this error:

        1: [ERR] Invalid JSON file. The file is not formatted according the schema. (code: 55 - JSON_SCHEMA_VALIDATION_ERROR)
                ./sub-07/ses-stand/motion/sub-07_ses-stand_task-dualWalking_tracksys-PhaseSpace1_motion.json
                        Evidence:  should NOT have additional properties

Remi-Gau · 2023-02-25T15:42:28Z

So having a task-dualWalking_motion.json with this content gives a valid bids dataset

{
	"TaskName":"dualWalking",
	"SamplingFrequency":480
}

Remi-Gau · 2023-02-25T15:46:27Z

With this file content

{
	"TaskName":"dualWalking"
}

We get this error which feels like too strict a requirement:
as long as a data file has all its required metadata AFTER applying the inheritance principle then the dataset should be valid.

There should not be a requirement for JSON files in the root directory to contain all required metadata.

        1: [ERR] Invalid JSON file. The file is not formatted according the schema. (code: 55 - JSON_SCHEMA_VALIDATION_ERROR)
                ./task-dualWalking_motion.json
                        Evidence:  should have required property 'SamplingFrequency'

This looks like a validator error. @rwblair any idea?

Remi-Gau · 2023-02-25T15:50:21Z

Also by trial and error it seems that the additional property error was due to the field:

MISCChannelCount

rwblair · 2023-03-09T15:11:52Z

Current bep029_motion validator branch seems to be working well with the example datasets. Confirmed locally as well.

sappelhoff · 2023-03-15T13:54:14Z

Okay, I am merging this one -- we can still work on this via PRs in the master branch. We'll tag a release only once the spec has been merged and released as well, which is still pending the channel type discussion

JuliusWelzel and others added 2 commits February 2, 2023 17:21

tmp: use motion branch for master validation

7939067

to be reverted before merging!

This was referenced Feb 2, 2023

BEP029 Motion (validator) bids-standard/legacy-validator#1434

Merged

[ENH] Extend BIDS for Motion data (BEP029) bids-standard/bids-specification#981

Merged

sjeung and others added 2 commits February 3, 2023 11:07

Remove old version of SR data set (#349)

3af761b

fix README

b0f825a

sappelhoff added the enhancement label Feb 8, 2023

sappelhoff commented Feb 8, 2023

View reviewed changes

motion_systemvalidation - update bidsignore

5d1c5b0

remove bidsignore entirely from systemvalidation

128f733

sjeung mentioned this pull request Feb 10, 2023

Bep029 motion dualtask #354

Merged

sappelhoff changed the base branch from master to bep021_ephys_derivatives February 11, 2023 18:33

sappelhoff changed the base branch from bep021_ephys_derivatives to master February 11, 2023 18:33

Merge branch 'master' into bep029_motion

08f7b06

sappelhoff changed the title ~~BEP029 Motion~~ BEP029 Motion (examples) Feb 20, 2023

sjeung and others added 2 commits February 21, 2023 12:03

[FIX] Fix column order in channels.tsv files (#358)

745e536

* [FIX] reorder columns to match schema * add latest changes * fix dataset name in readme * Remove repeating entries in scans.tsv --------- Co-authored-by: Julius Welzel <julius.welzel@gmail.com>

table fences

0c67d5a

guiomar reviewed Feb 21, 2023

View reviewed changes

motion_dualtask/participants.json Outdated Show resolved Hide resolved

motion_dualtask/participants.json Outdated Show resolved Hide resolved

...k/sub-07/ses-stand/motion/sub-07_ses-stand_task-dualWalking_tracksys-PhaseSpace1_motion.json Show resolved Hide resolved

Remi-Gau requested changes Feb 22, 2023

View reviewed changes

guiomar added 2 commits February 23, 2023 10:03

Update motion_dualtask/participants.json

41efd29

Include age range in "young"

Update motion_dualtask/participants.json

a051c1e

Include age range in "old"

Switch back to master for ci now that validator merged.

af0dd54

sappelhoff merged commit 2d8fe1e into master Mar 15, 2023

sappelhoff deleted the bep029_motion branch March 15, 2023 13:54

Conversation

sappelhoff commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Leads:

Links:

To do:

Uh oh!

sappelhoff commented Feb 2, 2023

Uh oh!

sappelhoff commented Feb 2, 2023

Uh oh!

JuliusWelzel commented Feb 3, 2023

Uh oh!

sappelhoff commented Feb 3, 2023

Uh oh!

sappelhoff commented Feb 8, 2023

Uh oh!

sjeung commented Feb 8, 2023

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

sappelhoff commented Feb 8, 2023

Uh oh!

rwblair commented Feb 8, 2023

Uh oh!

sappelhoff commented Feb 8, 2023

Uh oh!

rwblair commented Feb 8, 2023

Uh oh!

sappelhoff commented Feb 8, 2023

Uh oh!

JuliusWelzel commented Feb 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Remi-Gau left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjeung commented Feb 22, 2023

Uh oh!

sjeung commented Feb 22, 2023

Uh oh!

Remi-Gau commented Feb 22, 2023

Uh oh!

sjeung commented Feb 24, 2023

Uh oh!

Remi-Gau commented Feb 24, 2023

Uh oh!

Remi-Gau commented Feb 25, 2023

Uh oh!

Remi-Gau commented Feb 25, 2023

Uh oh!

Remi-Gau commented Feb 25, 2023

Uh oh!

Remi-Gau commented Feb 25, 2023

Uh oh!

rwblair commented Mar 9, 2023

Uh oh!

sappelhoff commented Mar 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sappelhoff commented Feb 2, 2023 •

edited

Loading

Remi-Gau left a comment •

edited

Loading