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

[ENH] relax ieeg channel name requirements of letters and numbers only #182

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

sappelhoff
Copy link
Member

related to the discussion in bids-standard/bids-examples#151

I also changed MUST to MAY regarding the specification of the reference channel, because this is not required.

@robertoostenveld
Copy link
Collaborator

+1

@choldgraf
Copy link
Collaborator

I'm +1 on this. Can anybody think of a case where we'd introduce problems by allowing symbols? I could see it as helpful in parsing names.

@robertoostenveld
Copy link
Collaborator

robertoostenveld commented Mar 19, 2019 via email

@sappelhoff
Copy link
Member Author

Please note that the channel names should also be stored in the EEG/iEEG/MEG data files.

yes - valid point! However, I think that this becomes clear from the text in the spec - if you want to make it more clear, a new PR could be the way to go.

PS: For future reviews, it'd be nice if you (Chris and Robert) could use the Github review tool to "approve" instead of commenting "+1" ... just so that we can act according to our decision making rules

Copy link
Member

@dorahermes dorahermes left a comment

Choose a reason for hiding this comment

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

LGTM

@choldgraf
Copy link
Collaborator

IMO we can cross the bridge of emoji channel names when we come to it :-)

@sappelhoff I wanted to hear some feedback about potential downsides before officially "approving" it ;-) this LGTM though

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 approve of the suggested changes.

@sappelhoff sappelhoff merged commit 70d907a into bids-standard:master Mar 25, 2019
@sappelhoff sappelhoff deleted the relax_requirement_ieeg branch March 25, 2019 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants