-
Notifications
You must be signed in to change notification settings - Fork 161
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
WIP: Overhaul CoordinateSystem details for MRI, MEG, EEG, iEEG #700
Conversation
also clarify that EEG / MEG can share their keywords if recorded simultaneously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple objections. First, I don't think we want so many options for coordinate units in MRI. Second, I think SpatialReference
will be a better tool than IntendedFor
.
Otherwise, these seem like fine proposals.
| **Key name** | **Requirement level** | **Data type** | **Description** | | ||
| --------------------------------- | --------------------- | ------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| AnatomicalLandmarkCoordinates | RECOMMENDED | [object][] of [arrays][] | Key:value pairs of any number of additional anatomical landmarks and their coordinates in units as specified in `AnatomicalLandmarkCoordinateUnits` (defaults to `"voxels"`, where first voxel has index 0,0,0) relative to the associated anatomical MRI (for example, `{"AC": [127,119,149], "PC": [128,93,141], "IH": [131,114,206]}`, or `{"NAS": [127,213,139], "LPA": [52,113,96], "RPA": [202,113,91]}`). Each array MUST contain three numeric values corresponding to x, y, and z axis of the coordinate system in that exact order. | | ||
| AnatomicalLandmarkCoordinateUnits | RECOMMENDED | [string][] | Can be `"mm"`, `"cm"`, `"m"`, or `"voxels"`, but defaults to `"voxels"` if not specified. If the value if `"voxels"`, the first voxel has index 0,0,0. This is the unit in which `AnatomicalLandmarkCoordinates` should be interpreted. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm extremely hesitant to allow anything besides voxels and mm. Particularly cm, which is not a valid NIfTI unit. I can't see meters being useful in any animal.
If non-voxel units are used, should we insist that the NIfTI xyzt_units
field match units? For instance, if somebody specifies mm
here, it might be sensible not to allow them to define their image affine in microns.
Additional changes:
- Typo. if -> is
- voxels to voxel. I did a quick search and it's hard to find clear guidance, but I think when describing a unit without a quantity, the singular is usually used. Happy to be corrected, if a reference can be found.
| AnatomicalLandmarkCoordinateUnits | RECOMMENDED | [string][] | Can be `"mm"`, `"cm"`, `"m"`, or `"voxels"`, but defaults to `"voxels"` if not specified. If the value if `"voxels"`, the first voxel has index 0,0,0. This is the unit in which `AnatomicalLandmarkCoordinates` should be interpreted. | | |
| AnatomicalLandmarkCoordinateUnits | RECOMMENDED | [string][] | Can be `"mm"`, `"cm"`, `"m"`, or `"voxel"`, but defaults to `"voxel"` if not specified. If the value is `"voxel"`, the first voxel has index 0,0,0. This is the unit in which `AnatomicalLandmarkCoordinates` should be interpreted. | |
| AnatomicalLandmarkCoordinates | RECOMMENDED | [object][] of [arrays][] | Key:value pairs of any number of additional anatomical landmarks and their coordinates in voxel units (where first voxel has index 0,0,0) relative to the associated anatomical MRI (for example, `{"AC": [127,119,149], "PC": [128,93,141], "IH": [131,114,206]}`, or `{"NAS": [127,213,139], "LPA": [52,113,96], "RPA": [202,113,91]}`). Each array MUST contain three numeric values corresponding to x, y, and z axis of the coordinate system in that exact order. | | ||
| **Key name** | **Requirement level** | **Data type** | **Description** | | ||
| --------------------------------- | --------------------- | ------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| AnatomicalLandmarkCoordinates | RECOMMENDED | [object][] of [arrays][] | Key:value pairs of any number of additional anatomical landmarks and their coordinates in units as specified in `AnatomicalLandmarkCoordinateUnits` (defaults to `"voxels"`, where first voxel has index 0,0,0) relative to the associated anatomical MRI (for example, `{"AC": [127,119,149], "PC": [128,93,141], "IH": [131,114,206]}`, or `{"NAS": [127,213,139], "LPA": [52,113,96], "RPA": [202,113,91]}`). Each array MUST contain three numeric values corresponding to x, y, and z axis of the coordinate system in that exact order. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Voxel index is well-defined, but I think it would be prudent to indicate that non-voxel distances increase to the right, anterior and superior. I could imagine someone seeing an LPS image and thinking that the distances are also LPS.
session-specific labels for example, "NAS-session1": `[127,213,139]`,"NAS-session2": | ||
`[123,220,142]`. | ||
session-specific labels for example, "NAS-session1": `[11.9,21.3,13.9]`,"NAS-session2": | ||
`[12.7,21.5,13.7]`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would add a note here that these would need AnatomicalLandmarkCoordinates
to be defined to interpret as distances from the origin rather than voxel locations.
- `Image`: Use this value in conjunction with the `IntendedFor` field. | ||
For example, if all coordinates you supply are to be interpreted with | ||
respect to a specific anatomical T1w image, specify `<datatype>CoordinateSystem` | ||
as `Image`, and `IndendedFor` with the path to that image in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as `Image`, and `IndendedFor` with the path to that image in the | |
as `Image`, and `IntendedFor` with the path to that image in the |
New fields shouldn't be described only in appendices. This also seems like a reversal of the usual way that IntendedFor
is used. The fieldmap is an adjunct to the image it is intended for. Here the anatomical appears to be the adjunct of the MEG/EEG data that are "intended for" it. What about co-opting the SpatialReference
field from derivatives?
Thanks a lot for your feedback @effigies. I investigated the I get the feeling that the different I think this should be its own issue - for the sake of this PR, I will see how much "easy improvement" I can salvage without touching/impacting in particular, we should clarify that IntendedFor contains a filepath (str) or list of filepaths ... and how these filepaths are to be made up ... if absolute or relative ... to what reference if relative, etc. see #471 Also I think all of those should accept a str or array of str (currently some only accept str). Lastly, the "descriptions" should be homogenized ... and then the special aspect of using "IntendedFor" with a particular usecase can be added at the end of the description ... or something similar. Just to get rid of this currently confusing diversity in descriptions. linking fieldmap data (Recommended, str or array of str)
link ASL timeseries and m0 scan (Required, str or array or str)
MEG: link MRI(s) (Optional, str or array of str)
EEG: Link MRI/CT (Optional, str)
iEEG: Link MRI7CT/Photograph (Recommended, str)
|
closing for now, as all urgent points have been addressed in #717 - all remaining discussion can continue in due time - drawing on the help page I created here: https://github.com/bids-standard/bids-specification/wiki/Coordinate-Systems-for-MEG-EEG-iEEG |
closes #661
History: Several months ago we improved the validator to finally check the
*CoordinateSystem
fields in (i)MEEG*coordsystem.json
files (https://github.com/bids-standard/bids-validator/pull/1096, https://github.com/bids-standard/bids-validator/pull/1097)This revealed issues with several existing bids-datasets (bids-standard/bids-examples#215), where dataset curators did not follow the rules outlined in the specification appendix VIII. The formalization of those rules in the bids-validator merely revealed that.
Several discussions later, we know several problems that need to be fixed (#520, #661, https://github.com/bids-standard/bids-specification/wiki/Coordinate-Systems-for-MEG-EEG-iEEG).
In this PR I propose some fixes:
UPDATE: Many of the proposed fixes that were uncontroversial have been merged in #717 ... the remaining changes are left here for potential discussion later.
New
Image
keywordOccasionally, people wanted to say their coordinates are to be interpreted with respect to some image, and then specified for example
T1w
as the CoordinateSystem, and pointed to that image using theIntendedFor
field.T1w
is currently not an accepted CoordinateSystem keyword. Instead of adding it to the list, I suggest a genericImage
keyword. Users can specifyImage
as a value to CoordinateSystem if they supply a path to an image in the bids dataset using theIntendedFor
field (this becomes REQUIRED in that case). Users could also point to a CT, or whatever image they deem sensible/meaningful.Allow units other than "voxel" for MRI
AnatomicalLandmarkCoordinates
In MRI, users can specify a field
AnatomicalLandmarkCoordinates
in voxels. This field can also be specified in MEG, EEG, and iEEG --> but there it's in mm, cm, or m.With this PR I introduce
AnatommicalLandmarkCoordinateUnits
to the MRI spec, where users can specify mm, cm, m, or voxels. If that field is not specified, it defaults to voxels - so this is a completely backward compatible change. In the future we may consider RECOMMENDING to not use voxels.for context see also #661 (comment)