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

tests for iEEG #79

Closed
jasmainak opened this issue Sep 7, 2018 · 6 comments · Fixed by #118
Closed

tests for iEEG #79

jasmainak opened this issue Sep 7, 2018 · 6 comments · Fixed by #118
Assignees
Labels
Milestone

Comments

@jasmainak
Copy link
Member

Currently the iEEG conversion is not at all tested.

cc @choldgraf

@jasmainak jasmainak added the maint label Sep 7, 2018
@teonbrooks teonbrooks added this to the 0.1.0 release 🎉 milestone Oct 1, 2018
@jasmainak
Copy link
Member Author

@choldgraf do you have any suggestion what data we should use for the testing here?

@choldgraf
Copy link
Contributor

Whenever the iEEG spec gets merged there should be some sample datasets released as well, we could just use those (they're lightweight because they don't have the actual data). Or we could just define our own dataset

@jasmainak
Copy link
Member Author

we do need actual data because mne-bids needs header information to write the json files. But on further thought, for the tests, we could probably make do with doing raw.set_channel_types and changing it to ecog. @monkeyman192 or @sappelhoff does any of you have time to take a stab at this?

@sappelhoff
Copy link
Member

I can give it a shot, but we will definitely need a full pass of mne-bids (preferably by @choldgraf) to integrate iEEG properly.

Note that iEEG will probably also support the NWB and MEF3 file formats, which are not yet implemented in MNE-Python. So we won't be able to cover these for now.

@jasmainak
Copy link
Member Author

iEEG is already implemented. So, it would be good to add just the tests (and any small related fixes). I would ideally not want to have it conflict too much with #99 and #106 . Thanks a lot for taking a shot at it !

@jasmainak
Copy link
Member Author

I agree we'll need a PR which does a full-pass down the line, but I would say it's not a blocker for our release at the moment. If we have the tests, I'm happy for now :)

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

Successfully merging a pull request may close this issue.

4 participants