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

[FIX] Clarify name of "BrainVision" format #175

Merged
merged 4 commits into from
Mar 22, 2019

Conversation

JegouA
Copy link
Contributor

@JegouA JegouA commented Mar 12, 2019

As discussed in the issue #172, I suggest changing the name BrainVision data format in BrainVision Core file data format in iEEG and EEG specifications.

closes #172

@JegouA JegouA requested a review from sappelhoff as a code owner March 12, 2019 12:06
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @JegouA :-)

sugestion: perhaps change the name to BrainVision Data Exchange Core Format like it says on their website? --> https://www.brainproducts.com/productdetails.php?id=21&tab=5

@sappelhoff
Copy link
Member

also, I believe that according to our new rules as listed in DECISION-MAKING, you have to add yourself to this file: https://github.com/bids-standard/bids-specification/blob/master/CODEOWNERS

Finally, after this PR is merged, you can make a new PR adding yourself as a contributor to BIDS in this file: https://github.com/bids-standard/bids-specification/blob/master/src/99-appendices/01-contributors.md

Sorry if that seems like a lot of overhead for the tiny changes. But perhaps you can see it as an opportunity and entry point for future contributions instead :-)

@sappelhoff sappelhoff changed the title Changing the name of BrainVision format [FIX] Clarifying the name of the "BrainVision" format Mar 12, 2019
@chrisgorgo
Copy link
Contributor

My reading of the rules was that adding yourself to 01-contributors.md and CODEOWNERS is optional, but if one chooses to they need to add their name to both (not just 01-contributors.md). Maybe we need to clarify this language.

@JegouA
Copy link
Contributor Author

JegouA commented Mar 13, 2019

Ok, I'm lost :). Do I have to add myself as a contributor to have my request accepted? Or could it be accepted anyway?

@sappelhoff
Copy link
Member

I just re-read the DECISION-MAKING.md file and yes - adding oneself as a contributor is optional ... but if one decides to do that, one also needs to add oneself as a CODEOWNER.

for the present issue: @JegouA it would be completely fine for you to incorporate my feedback and then we can merge this PR in a few days (if at least one more BIDS person agrees that this is helpful)

@sappelhoff
Copy link
Member

@JegouA do you see the two suggestions I made to your code? If you accept them, we are ready to merge this.

I suggest to use BrainVision Core Data Format, because it corresponds with Brain Products website: https://www.brainproducts.com/productdetails.php?id=21&tab=5

PS: They have updated their examples, and there is no mention of .dat anymore :-)

sappelhoff and others added 2 commits March 22, 2019 12:10
Co-Authored-By: JegouA <45284001+JegouA@users.noreply.github.com>
…ography.md

Co-Authored-By: JegouA <45284001+JegouA@users.noreply.github.com>
@sappelhoff
Copy link
Member

Thanks!

Please feel free to make a new Pull Request to add yourself as a contributor (if you want)!

That is, you can add your name here: 01-contributors.md

@JegouA
Copy link
Contributor Author

JegouA commented Mar 22, 2019

@sappelhoff sorry, I didn't know that I have to accept the changes

@sappelhoff sappelhoff changed the title [FIX] Clarifying the name of the "BrainVision" format [FIX] Clarify name of "BrainVision" format Mar 22, 2019
@sappelhoff sappelhoff merged commit 54efc0e into bids-standard:master Mar 22, 2019
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.

iEEG specification for BrainVision format
3 participants