-
Notifications
You must be signed in to change notification settings - Fork 169
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] Propose to use *_channels.json
for the description of coordinate systems
#1524
Conversation
for more information, see https://pre-commit.ci
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.
we also need to remove the references to RotationRule
, RotationOrder
, and SpatialAxes
here:
bids-specification/src/schema/rules/sidecars/motion.yaml
Lines 74 to 75 in 8c9fa11
RotationOrder: recommended | |
RotationRule: recommended |
Else they'll keep showing up as recommended fields in motion.json
.
Instead we need to add some schema magic to make them show up under channels.json
IF the modality is motion. cc @tsalo @effigies
furthermore, I think the reference_frame
column for channels.tsv
should also be mentioned here:
bids-specification/src/modality-specific-files/motion.md
Lines 140 to 144 in 8c9fa11
This file is REQUIRED as it makes it easy to browse or query over larger collections of datasets. | |
The REQUIRED columns are channel `name`, `component`, `type`, `tracked_point` and `units`. | |
Any number of additional columns MAY be added to provide additional information about the channels. | |
The `*_tracksys-<label>_channels.tsv` file SHOULD give additional information about individual recorded channel, | |
some of which my not be found summarized in `*_motion.json`. |
and link to the new section that you are introducing ("reference frame description")
|
||
### Example of `*_channels.json` | ||
|
||
```json |
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.
would be nice to have a short TSV example with "reference_frame" "local" somewhere. Either a new one here or in the existing example above.
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.
I only did one example for now. In my experience that is less complicated, than providing multiple ones. It is mentioned in the text, that the reference_frame
column is only RECOMMENDED.
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1524 +/- ##
=======================================
Coverage 87.83% 87.83%
=======================================
Files 16 16
Lines 1356 1356
=======================================
Hits 1191 1191
Misses 165 165 ☔ View full report in Codecov by Sentry. |
I implemented @sappelhoff comments in the previous commits. @sjeung can you please check if my suggestion for introducing the |
Hi! Both the example and the description in the specification look good from my side. @JuliusWelzel could you remind me if your example data set is up-to-date? For validator I only removed the fields from motion.json. |
Started to comment here, just made edits instead: #1591 Diff from this PR: https://github.com/JuliusWelzel/bids-specification/compare/master...effigies:bids-specification:channels-json |
I don't know how to validate that in the legacy validator. In the schema validator, you can't currently validate anything inside column descriptions. I'm not sure what the best path forward for that would be. (Flagging @rwblair, as we'll need to think about this.) |
@sappelhoff This PR should be merged into |
@JuliusWelzel Could you review #1591? It contains these proposals: |
Looks good to me. Will will update the Examples accordingly. Thanks! |
|
Hello,
as discussed before, we would like to move the description of coordinate system information for motion data from the
*_motion.json
to the*_channels.json
file.@sjeung already implemented this in her example datasets, I will not get around to do this before my holiday :(
We updated the schema accordingly.
Please check if the PR seems ok.
Best,
Julius