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] Propose to use *_channels.json for the description of coordinate systems #1524

Closed
wants to merge 12 commits into from

Conversation

JuliusWelzel
Copy link
Collaborator

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

@sappelhoff sappelhoff added this to the 1.9.0 milestone Jun 27, 2023
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.

we also need to remove the references to RotationRule, RotationOrder, and SpatialAxes here:

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:

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
Copy link
Member

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.

Copy link
Collaborator Author

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
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (fc98780) 87.83% compared to head (964705a) 87.83%.
Report is 42 commits behind head on master.

❗ Current head 964705a differs from pull request most recent head 98112eb. Consider uploading reports for the commit 98112eb to get more accurate results

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.
📢 Have feedback on the report? Share it here.

@JuliusWelzel
Copy link
Collaborator Author

I implemented @sappelhoff comments in the previous commits. @sjeung can you please check if my suggestion for introducing the reference_frame column is sufficient for now?

sjeung added a commit to sjeung/bids-validator that referenced this pull request Aug 22, 2023
@sjeung
Copy link
Collaborator

sjeung commented Aug 22, 2023

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?
My updated examples can be found here .

For validator I only removed the fields from motion.json.
@tsalo @effigies @sappelhoff, Do you have any suggestions on whether to validate the channels.json according to this part of the spec?

@effigies
Copy link
Collaborator

effigies commented Aug 22, 2023

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

@effigies
Copy link
Collaborator

Do you have any suggestions on whether to validate the channels.json according to this part of the spec?

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.)

@JuliusWelzel
Copy link
Collaborator Author

JuliusWelzel commented Oct 3, 2023

@sappelhoff This PR should be merged into main before the release as it clarifies how to describe coordinate systems for motion data. Is there anything we can do to help speed up the process?

@effigies
Copy link
Collaborator

effigies commented Oct 3, 2023

@JuliusWelzel Could you review #1591? It contains these proposals:

c00d1c1...6ab3e00

@JuliusWelzel
Copy link
Collaborator Author

@JuliusWelzel Could you review #1591? It contains these proposals:

c00d1c1...6ab3e00

Looks good to me. Will will update the Examples accordingly. Thanks!

@JuliusWelzel
Copy link
Collaborator Author

Will will update the Examples accordingly.

Examples have been updated here, I think it is ready to be merged.

Minor changes to specs have been made as well

@sappelhoff
Copy link
Member

@sappelhoff sappelhoff closed this Oct 24, 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.

4 participants