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] clarify XXXCoord* in the coordinate systems appendix #520

Merged
merged 3 commits into from
Aug 27, 2020

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Jul 1, 2020

closes #493

benefits

to discuss

T1w as a restricted keyword

see: https://github.com/bids-standard/bids-examples/issues/190

inconsistencies between *CoordinateSystems (note the * wildcard)

In the EEG AND MEG, sections on coordinate systems, we clearly sate that the <datatype>CoordinateSystem fields are supposed to be filled with values from a restricted list provided in appendix 7

Example from EEG

REQUIRED. Refers to the coordinate system in which the EEG electrode positions are to be interpreted (see Appendix VIII).

Yet for all other <datatype>*CoordinateSystem fields (like FiducialsCoordinateSystem and AnatomicalLandmarkCoordinateSystem in EEG ... OR HeadCoilCoordinateSystem and DigitizedHeadPointsCoordinateSystem in MEG), we do not say that:

should this text be adjusted consistently across the *CoordinateSystems?

note: for iEEG this seems to be no problem, because there is only iEEGCoordinateSystem

@sappelhoff sappelhoff added EEG Electroencephalography help wanted Extra attention is needed MEG Magnetoencephalography labels Jul 2, 2020
@sappelhoff
Copy link
Member Author

I realize that in this PR I did a curious combination of:

  1. providing some straight forward fixes
  2. starting a difficult discussion

I should split these two and get the straight forward fixes merged. The discussion will take time and people.

src/99-appendices/08-coordinate-systems.md Outdated Show resolved Hide resolved
src/99-appendices/08-coordinate-systems.md Outdated Show resolved Hide resolved
src/99-appendices/08-coordinate-systems.md Outdated Show resolved Hide resolved
src/99-appendices/08-coordinate-systems.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. Would be good to get some other @bids-standard/raw-electrophys eyes on it.

One small worry...

`coordsystem.json` file for EEG datasets:

- `Captrak`: RAS orientation and the origin between the ears
- `CapTrak`: RAS orientation and the origin between the ears
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a backwards incompatible change. Is it just a typo being fixed? Do we need to worry about someone taking it too seriously in the past?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, a typo fix. I haven't seen any datasets that use CapTrak yet --> and in fact, the list of restricted keywords for CoordSystems is not yet validated, see: bids-standard/bids-validator#945

due to that missing coverage in the validator we currently have several datasets on openneuro that specify their coordsystem as e.g., "ARS", which is invalid of course. (see e.g., https://openneuro.org/datasets/ds002680/versions/1.0.0)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, being specified as "ARS" we still don't know how to relate electrode positions to the anatomy of the head, whereas "Other" and some textual description could have given more information.

@robertoostenveld
Copy link
Collaborator

I agree with the proposed straight-forward changes to the specification. It might be that I was responsible for writing it as XXXCoordinateSystem, since I use that style of documenting more often (also in FieldTrip functions). Writing it as <xxx> is more consistent with BIDS documentation elsewhere.

@robertoostenveld
Copy link
Collaborator

The other point @sappelhoff raises is to "to discuss T1w as a restricted keyword" as e.g. in https://github.com/bids-standard/bids-examples/issues/190. In the specific example it is clear (to me) that the positions relate to a specific T1w MRI, but still unclear how to interpret the specified position of the subsequent landmarks in relation to the participant's anatomy. But then, the anatomical provide this relation, so as such I should not expect that the anatomical landmarks are expressed relative to any coordinate system other than that of the MRI.

Imagine the coordinatesystem.json would have specified

"AnatomicalLandmarkCoordinates": { 
     "LPA": [..., ..., ...], 
     "NAS": [..., ..., ...], 
     "RPA": [..., ..., ...] 
 }, 
 "AnatomicalLandmarkCoordinateSystem": "CTF",  <- note here
 "AnatomicalLandmarkCoordinateUnits": "mm", 
 "IntendedFor": "anat/sub-01_T1w.nii.gz" 

Then I would have known that the MRI is expressed in CTF coordinates and hence that the NAS would be along the positive x-axis.

With

"AnatomicalLandmarkCoordinates": { 
     "LPA": [..., ..., ...], 
     "NAS": [..., ..., ...], 
     "RPA": [..., ..., ...] 
 }, 
 "AnatomicalLandmarkCoordinateSystem": "Elekta/Neuromag", 
 "AnatomicalLandmarkCoordinateUnits": "mm", 
 "IntendedFor": "anat/sub-01_T1w.nii.gz" 

I would have known that NAS would be along the positive y-axis.

But imagine that it would say

"AnatomicalLandmarkCoordinates": { 
     "LPA": [..., ..., ...], 
     "NAS": [..., ..., ...], 
     "RPA": [..., ..., ...] 
 }, 
 "AnatomicalLandmarkCoordinateSystem": "Unknown", (or device, or whatever) 
 "AnatomicalLandmarkCoordinateUnits": "mm", 
 "IntendedFor": "anat/sub-01_T1w.nii.gz" 

I would not have a-priori expectations on the values of NAS/LPA/RPA, but I would still be able to do all analysis on the shared data: The AnatomicalLandmarkCoordinates tells me where the landmarks are, and if EEGCoordinateSystem were e.g. in CTF, I could relate the positions from the _electrodes.tsv via the AnatomicalLandmarkCoordinates and then via the coordinate system specified in the NIFTI file to the corresponding voxels in the NIFTI file.

The specification is confusing, but I think that is because matching coordinates (even after working 20 years in the field) is confusing. It is something I explain to (new) Donders colleagues about every month, over and over again. I am not sure how we should resolve this confusion, but feel that explaining it (e.g. in the starter kit, or on the FT website) is more appropriate than trying to make the explanation part of the specification.

Having said that: I don't see a problem in the specification as T1w. It could also be a CT or a T2w or whatever file the IntendedFor 3D scan is linking to. I think that by specifying a meaningful string here, we are trying to give some information about the 3D scan that was not provided with the 3D scan itself or in its metadata. The way I read the specification of T1w is "I don't know how to interpret the coordinate system from the NIFTI file, but this is where the anatomical landmarks are in the NIFTI file".

Rather than checking whether a specific example makes sense; what if we would try to identify which specifications seem to be currently allowed but are truly problematic ...

PS when reading up on details, I did notice a section in the MEG specification which states The solution would be to use only one T1w file and populate the AnatomicalLandmarkCoordinates field with session-specific labels e.g., "NAS-session1": [127,213,139],"NAS-session2": [123,220,142], etc.. These integer numbers are suggestive of the Coordinates being specifid as voxel indices. That should never be allowed, right?

@sappelhoff
Copy link
Member Author

sappelhoff commented Aug 25, 2020

am not sure how we should resolve this confusion, but feel that explaining it (e.g. in the starter kit, or on the FT website) is more appropriate than trying to make the explanation part of the specification.

okay

The way I read the specification of T1w is "I don't know how to interpret the coordinate system from the NIFTI file, but this is where the anatomical landmarks are in the NIFTI file".

yep, me too.

Rather than checking whether a specific example makes sense; what if we would try to identify which specifications seem to be currently allowed but are truly problematic

agreed, and that is best being done when we have a case at hand. I currently have none, so I am fine to postpone this discussion until I or somebody else brings it up again with concrete problems to solve.

PS when reading up on details, I did notice a section in the MEG specification which states The solution would be to use only one T1w file and populate the AnatomicalLandmarkCoordinates field with session-specific labels e.g., "NAS-session1": [127,213,139],"NAS-session2": [123,220,142], etc.. These integer numbers are suggestive of the Coordinates being specified as voxel indices. That should never be allowed, right?

is it possible to convert from MRI + voxelspace to some other coordinate system and another unit (e.g., mm)? --> if yes, then I could imagine allowing it. I imagine that when a researcher looks at an MRI and tries to find the fiducials (e.g., vitamin E pills) in the MRI to identify the landmarks, then noting down the voxels may be most straight forward?


Overall --> @robertoostenveld are you fine with this PR being merged and we postpone the discussion like I mentioned above?

Copy link
Collaborator

@robertoostenveld robertoostenveld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with merging the proposed changes. The parts of the discussion that have not been resolved need to be dealt with elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EEG Electroencephalography help wanted Extra attention is needed MEG Magnetoencephalography
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify XXXCoordinateSystem
3 participants