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

ieeg_epilepsy / ieeg_epilepsy_ecog: Fix CoordinateSystem #219

Closed
wants to merge 5 commits into from
Closed

ieeg_epilepsy / ieeg_epilepsy_ecog: Fix CoordinateSystem #219

wants to merge 5 commits into from

Conversation

ftadel
Copy link
Contributor

@ftadel ftadel commented Dec 5, 2020

related #215

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 seems like I made the impression that I was very strict and determined to have all keywords relating to the table from appendix VIII removed --> sorry for that.

I think this does not need to be fixed - instead I will open 2 PRs: one to BIDS-specification (see the plan discussion: bids-standard/bids-specification#661 (comment)) and one to the bids-validator that will make coordinate system definitions like the one here valid.

Once all of that is thoroughly discussed and implemented, we can revisit this dataset.

@sappelhoff
Copy link
Member

From your suite of PRs this is the only one that I can't merge right now. I am hopeful that with bids-standard/bids-specification#700 the change

- "iEEGCoordinateSystem": "MNI152",
+ "iEEGCoordinateSystem": "Other",

won't be necessary and we can keep MNI152, as it's also listed in the table here: https://bids-specification.readthedocs.io/en/stable/99-appendices/08-coordinate-systems.html#standard-template-identifiers

one change that'd have to be made though (if my PR goes through as it is currently) concerns the sub-ecog01_ses-postimp_space-other_coordsystem.json file, where the electrode coords are to be interpreted with regards to sub-ecog01/ses-postimp/anat/sub-ecog01_ses-postimp_T1w.nii.gz

there, instead of "Other" the keyword "Image" would be used if it goes according to the current state of the PR.

@sappelhoff
Copy link
Member

I am going ahead and will incorporate parts of your PR into #226.

Thanks!

@sappelhoff sappelhoff closed this Feb 1, 2021
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.

2 participants