bids-standard / bids-specification

Brain Imaging Data Structure (BIDS) Specification
https://bids-specification.readthedocs.io/
Creative Commons Attribution 4.0 International
270 stars 156 forks source link

"T1w" as keyword for <datatype>*CoordinateSystem #661

Open sappelhoff opened 4 years ago

sappelhoff commented 4 years ago

https://github.com/bids-standard/bids-examples/blob/23ab390fdfb4d38b3e3eeabdbd663e2ac09a16bc/eeg_ds000117/sub-01/eeg/sub-01_coordsystem.json#L2-L11

the T1w keyword seems to indicate that all coordinates must be interpreted in the light of the T1 weighted MRI image that is supplied with the IntendedFor field.

However, such a convention is currently not documented in Appendix 8: https://bids-specification.readthedocs.io/en/latest/99-appendices/08-coordinate-systems.html#coordinate-systems-applicable-to-meg-eeg-and-ieeg

sappelhoff commented 3 years ago

For those who are generally interested in coordinate systems in BIDS, I have created a wiki page that summarizes the status quo and also lists currently open questions and issues. --> https://github.com/bids-standard/bids-specification/wiki/Coordinate-Systems-for-MEG-EEG-iEEG

It's a WIKI, so please add and/or correct items!

adam2392 commented 3 years ago

iEEG user here: I think it would be very helpful to have T1w as a keyword for the CoordinateSystem because you generally end up localizing electrodes and mapping them to a T1w image. The current keywords essentially only account for if you ACPC align your T1w image before mapping ieeg coordinates to the image.

However, there are cases where this description imo is insufficient. For example, if I want to map iEEG coordinates onto the T1w defined from FreeSurfer, it seems that a T1w keyword with IntendedFor = T1.mgz (from FreeSurfer) would be more sensible.

sappelhoff commented 3 years ago

you generally end up localizing electrodes and mapping them to a T1w image.

so are the electrode coordinate units then in "voxels"? or still in "mm", but mapped on the T1w image?

Perhaps it'd make sense to do something like:

also, let's not forget what @robertoostenveld said in his comment here: https://github.com/bids-standard/bids-specification/pull/520#issuecomment-672757321

robertoostenveld commented 3 years ago

if it were expressed in voxels, it would also have to be made explicit whether voxel indices are counted with a 0-offset or with a 1-offset. I.e., is the first corner of the volume [0,0,0] or [1,1,1]? If it were expressed in mm, that would not be needed, as it is taken care of by the low-level NIFTI file definition.

sappelhoff commented 3 years ago

if it were expressed in voxels, it would also have to be made explicit whether voxel indices are counted with a 0-offset or with a 1-offset. I.e., is the first corner of the volume [0,0,0] or [1,1,1]?

indeed, and for the MRI AnatomicalLandmarkCoordinates field, 0-indexing is prescribed: https://bids-specification.readthedocs.io/en/latest/04-modality-specific-files/01-magnetic-resonance-imaging-data.html#anatomical-landmarks

dorahermes commented 3 years ago

We typically express coordinates in mm, but mapped onto a T1 image.

iEEGCoordinateUnits currently does not have the option for voxels.

sappelhoff commented 3 years ago

Taken from @robertoostenveld's comment: https://github.com/bids-standard/bids-specification/pull/520#issuecomment-672757321

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 I agree with this, but it also renders T1w (or T2w, CT, ...) non-helpful. I think instead it could/should be something generic like Image. and if Image is specified as *CoordinateSystem, then an IntendedFor MUST be specified that links to the image file that defines the reference of electrodes.tsv.

WDYT?

also, if you are reading this, please also check out https://github.com/bids-standard/bids-examples/issues/215

adam2392 commented 3 years ago

I think I agree with this, but it also renders T1w (or T2w, CT, ...) non-helpful. I think instead it could/should be something generic like Image. and if Image is specified as *CoordinateSystem, then an IntendedFor MUST be specified that links to the image file that defines the reference of electrodes.tsv.

WDYT?

This makes sense to me and imo is more informative then just "Other" which is currently how one encodes any arbitrary coordinate system for iEEG. Allowing for CT is also important since technically your ieeg coordinates are usually localized on that image.

sappelhoff commented 3 years ago

Allowing for CT is also important since technically your ieeg coordinates are usually localized on that image.

With the proposal from https://github.com/bids-standard/bids-specification/issues/661#issuecomment-739317079 you would encode CT by supplying Image as the CoordinateSystem and then point to a CT file with IntendedFor. Of course, that CT file would have to be put into .bidsignore until CT support is officially part of BIDS.

Coming back to:

in that case, units MAY also be in voxels ... otherwise they must be in mm, cm, or m

That wouldn't work for CT would it? Or is CT also measured in voxels? I get the impression that we should not support voxels for this after all ... and perhaps deprecate the only place where it's currently allowed (MRI AnatomicalLandmarkCoordinates field)

dorahermes commented 3 years ago

I think that allowing Image and IntendedFor and having coordinates in mm units would suffice for iEEG (also for a CT in .bidsignore).

In terms of processing, we get either mm coordinates or voxel indices from a CT.nii.gz and there is just 1 matrix transformation between these two. For consistency, it is not necessary to allow a new unit (voxels) for iEEG and mm/cm/m is sufficient.

sappelhoff commented 3 years ago

I am planning to open a PR soon that will address (1) an Image keyword to be used in conjunction with IntendedFor, and (2) allowing the keywords like Talairach etc. from the table.

However, I have some questions/comments about the intro text to the table:

The transformation of the real world geometry to an artificial frame of reference is described in CoordinateSystem. Unless otherwise specified below, the origin is at the AC and the orientation of the axes is RAS. Unless specified explicitly in the sidecar file in the CoordinateUnits field, the units are assumed to be mm.

  1. The first sentence will need some clarification - in particular that the keywords listed in the table are valid entries to *CoordinateSystem.
  2. the second sentence says that "the origin is at the AC and the orientation of the axes is RAS" --> is the AC a point in space? I thought that was more of a "line"? --> furthermore are there entries in the table at all that do not specify origin and axes orientations? ... I think I'd prefer it if each of the listed coordinate systems would clearly define the origin and axes orientations.
  3. the third question states that the default value for CoordinateUnits is mm. However, given that CoordinateUnits is a mandatory field that only accepts mm, cm, and m, I think we can cut that sentence.

In terms of processing, we get either mm coordinates or voxel indices from a CT.nii.gz and there is just 1 matrix transformation between these two. For consistency, it is not necessary to allow a new unit (voxels) for iEEG and mm/cm/m is sufficient.

okay, +1 to not use voxels then, and I'll think about deprecating using "voxels" as a unit in the MRI AnatomicalLandmarkCoordinates for consistency in the spec.

sappelhoff commented 3 years ago

see https://github.com/bids-standard/bids-specification/pull/700

ftadel commented 3 years ago

+1 for allowing space-Image, and enforcing IntendedFor in this case.

To avoid any ambiguity, we could limit this use case to the scanner-based coordinates, as defined in the qform matrix of the .nii file (eg. the coordinates we see in the title bar of MRIcron). Since both the NII and MGZ file formats provide well documented solutions to define one unique 4x4 transformation matrix between these scanner coordinates in mm and the voxels, I'm not sure we need to add any additional flexibility.

"space-Other" + iEEGCoordinateSystemDescription would lead to a lot of inconsistencies across datasets, and interpretation problems.

Examples: https://github.com/bids-standard/bids-examples/blob/master/ieeg_epilepsy_ecog/sub-ecog01/ses-postimp/ieeg/sub-ecog01_ses-postimp_space-Other_coordsystem.json https://github.com/bids-standard/bids-examples/blob/master/ieeg_epilepsy/sub-01/ses-postimp/ieeg/sub-01_ses-postimp_space-Other_coordsystem.json

robertoostenveld commented 3 years ago

is the qform in the NII file always in scanner-based coordinates?

ftadel commented 3 years ago

is the qform in the NII file always in scanner-based coordinates?

Well, it can be anything because it can be edited after acquisition. But we can call it "a scanner-based" coordinate system. And it has the advantage of almost always being defined, and unique. It is proper to a single file, and not comparable with other files even in the same study, but it doesn't matter.

https://nifti.nimh.nih.gov/nifti-1/documentation/nifti1fields/nifti1fields_pages/qsform_brief_usage