-
Notifications
You must be signed in to change notification settings - Fork 109
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
[MRG] Add coordsystem eeg #533
[MRG] Add coordsystem eeg #533
Conversation
Time for a short review @robertoostenveld ? This is pretty straight forward though, so I think it can be merged @chrisfilo |
"AnatomicalLandmarkCoordinateUnits": { "type": "string", "minLength": 1 }, | ||
"AnatomicalLandmarkCoordinateSystemDescription": { "type": "string" } | ||
}, | ||
"required": ["EEGCoordinateSystem", "EEGCoordinateUnits"], |
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.
electrode positions are not always present (e.g. clinical EEG data, only standard 10-20 electrode names), hence I think that the coordinate system should not be required, only recommended.
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.
Just traveling back from BIOMAG, where many groups presented pilot results from OPMs. It could very well be that also for OPM-based MEG systems we will get the situation that channel positions are not numerically specified.
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.
The "required" key just kicks in IF there is a coordsystem_eeg.json. And if we do have coordinates, there should be metadata on the system and its units.
If no coordsystem.json is specified, this json schema is not loaded and thus, nothing will be required.
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.
perfect, thanks!
I don't really know what you mean by OPM, but it sounds like something to be discussed in our Spec. Thanks for the review. |
OPM stands for Optically Pumped Magnetometer, which is a new type of MEG sensor. See here for a recent review https://www.nature.com/articles/nature26147. Rigid 3D printed arrangements are now mostly used, but flexible OPM caps (like EEG caps) are already being piloted. |
@chrisfilo, please merge or object :-) |
so far, the eeg coordsystem definition was squashed into the meg coordsystem using "anyOf". This is now fixed by introducing an eeg specific file, adjusting the meg file, and making the appropriate calls in validators/json.js