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

[FIX] Add coordsystem-specific definition of DigitizedHeadPoints #1023

Merged
merged 3 commits into from
Apr 6, 2022

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Mar 4, 2022

Closes #1022.

Changes proposed:

  • Add definition for DigitizedHeadPoints for coordsystem files.

@tsalo tsalo added the schema Issues related to the YAML schema representation of the specification. Patch version release. label Mar 4, 2022
@tsalo
Copy link
Member Author

tsalo commented Mar 4, 2022

@Moo-Marc would you mind taking a look at the proposed changes?

@Moo-Marc
Copy link
Contributor

Moo-Marc commented Mar 4, 2022

Thanks,
The contents looks good, though I'm not familiar with the syntax.

@tsalo
Copy link
Member Author

tsalo commented Mar 4, 2022

@Moo-Marc thanks for taking a look. Here is the updated definition, for posterity:

image

@sappelhoff do you know what kind of relative path DigitizedHeadPoints should be? I put participant_relative, like IntendedFor, but I don't know if that's correct. The definition is a little vague.

@sappelhoff
Copy link
Member

good question, I don't know :-(

in this example, it's apparently relative to the coordsystem.json file itself (where the DigitizedHeadPoints metadata is specified): https://github.com/bids-standard/bids-examples/blob/df982afc719751d4b7f38d426aa38fc6832e06fa/ds000117/sub-01/ses-meg/meg/sub-01_ses-meg_coordsystem.json#L8

that looks super odd and is probably a bug. Another reason why we need #918

Maybe @Moo-Marc knows? Or @robertoostenveld?

@Moo-Marc
Copy link
Contributor

Moo-Marc commented Mar 5, 2022

I was a bit confused about paths as well and following the examples. So for this I used the file name only (it's in the same folder). I like the idea of #918.

@robertoostenveld
Copy link
Collaborator

I suspect that back in the days of writing the MEG specification the idea was to have this similar as IntendedFor with a past specification that is relative to the JSON file that specifies it. So an example without explicit path like "sub-01_headshape.pos" would be in the same directory as the coordinatesystem.json

@sappelhoff
Copy link
Member

I suspect that back in the days of writing the MEG specification the idea was to have this similar as IntendedFor with a past specification that is relative to the JSON file that specifies it.

Thanks Robert, I think you are right.

@tsalo would that mean that we need a new "format" for paths? Currently we just have dataset_relative and participant_relative:

- `dataset_relative` (relative paths from dataset root),
- `participant_relative` (relative paths from participant folder).

of course, all of these will be DEPRECATED anyhow once we merge #918 ... but we still need to solve this issue for backward compatibility

@tsalo
Copy link
Member Author

tsalo commented Mar 29, 2022

@sappelhoff I think you're right. Since it should be relative to the file in which the field is present, does file_relative sound good?

@sappelhoff
Copy link
Member

does file_relative sound good?

👍 sounds good to me!

@tsalo
Copy link
Member Author

tsalo commented Apr 5, 2022

It's now set to file_relative.

@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.50%. Comparing base (ee9e8b5) to head (97c75b8).
Report is 1426 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1023      +/-   ##
==========================================
+ Coverage   70.53%   71.50%   +0.96%     
==========================================
  Files           9        9              
  Lines         930      930              
==========================================
+ Hits          656      665       +9     
+ Misses        274      265       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

I think this is good to go then. Thanks a bunch @tsalo (and @Moo-Marc for noticing)

@sappelhoff sappelhoff merged commit 0b54380 into bids-standard:master Apr 6, 2022
@tsalo tsalo deleted the fix-digitizedheadpoints branch April 7, 2022 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
schema Issues related to the YAML schema representation of the specification. Patch version release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MEG DigitizedHeadPoints definitions
4 participants