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

we should probably restrict the kinds of "labels" that can be passed to entity: *_space-<label> #709

Closed
sappelhoff opened this issue Feb 15, 2021 · 9 comments · Fixed by #724
Milestone

Comments

@sappelhoff
Copy link
Member

sappelhoff commented Feb 15, 2021

We should probably restrict the kinds of "labels" that can be passed to entity: *_space-<label>

Currently, any string can be passed ... this passes the validator, because the validator does not cover this case currently (but may do so in the future).

came up in #677 (comment)

see bids-validator issue: https://github.com/bids-standard/bids-validator/issues/743
see bids spec clarification PR: https://github.com/bids-standard/bids-specification/pull/734/files

note that although I crossreference to a clarification PR in the BIDS spec, the spec always contained the sentence that the label for the *_space-<label> should be taken from a list of permissible keywords.

@hoechenberger hoechenberger added this to the 0.7 milestone Feb 15, 2021
@sappelhoff
Copy link
Member Author

@hoechenberger
Copy link
Member

So … which space should be added in the concrete (failing) example?
@adam2392 Can you have a look at this?

@adam2392
Copy link
Member

adam2392 commented Feb 25, 2021

I think the issue is that now anything with coordframe == 'mri' in mne-python -> "Other" and one must pass *CoordinateSystemDescription and should pass IntendedFor.

@sappelhoff can you help clarify what the below should be?

For iEEG mne-python -> BIDS:

fs_tal -> ? no idea
mni_tal -> fsaverage?
mri_voxel -> error, needs to convert to mm
ras -> ? no idea
head -> EEG Captrak (or error if it's ieeg)

@hoechenberger
Copy link
Member

and one must pass coordinatesystemdescription

HELP, is this supposed to be a keyword argument?? It makes my eyes bleed trying to read this monstrosity 😱 Can we come up with something considerably more concise? :)

@sappelhoff
Copy link
Member Author

Yes, "Other" if the Coordinate System is not "known" in BIDS.

Of course, if it is a well known coordinate system, then it can be added to BIDS without a log of hassle.

However, there is a huge amount of coordinate systems supported, see: https://bids-specification.readthedocs.io/en/latest/99-appendices/08-coordinate-systems.html#standard-template-identifiers

And I suspect that for some of these, the issue just boils down to not knowing the mapping.

fs_tal -> ? no idea

I don't know what that is either ... so probably "Other", and put some meaningful thing in "*CoordsystemDescription"

mni_tal -> fsaverage?

that'd be great if you could find out ... because fsaverage would be supported.

mri_voxel -> error, needs to convert to mm

and if we convert to mm (which should be straight forward), then what would the coord system be from the BIDS list?

ras -> ? no idea

ras is no coordinate system, it just tells you about the direction of the axes, but nothing about the origin of the coordinate system (and nothing about the units, which is a bit less important, but still).

head -> EEG Captrak (or error if it's ieeg)

We could make CapTrak permissible for iEEG as well, ... IF any ieeg people actually use this kind of coordinate system.

@sappelhoff
Copy link
Member Author

sappelhoff commented Feb 25, 2021

HELP, is this supposed to be a keyword argument?? It makes my eyes bleed trying to read this monstrosity scream Can we come up with something considerably more concise? :)

it's actually EEGCoordinateSystemDescription, and may I introduce you to my friend DigitizedHeadPointsCoordinateSystemDescription <3 (not a joke 😢 )

@hoechenberger
Copy link
Member

hoechenberger commented Feb 25, 2021

it's actually EEGCoordinateSystemDescription, and may I introduce you to my friend DigitizedHeadPointsCoordinateSystemDescription <3 (not a joke 😢 )

At least this is CamelCased SoItsNotAllThatBadIGuessButInTheEndItsReallyJustAMatterOfTasteRight?

But seriously, if we do decide to use these names, we should at least turn them into snake_case and not simply lowercase them :)

@adam2392
Copy link
Member

@hoechenberger apologies. Typing this out on GitHub mobile app heh :p

@sappelhoff
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants