[MRG] add ieeg tests

[sappelhoff]

closes #79

[codecov-io]

Codecov Report

Merging #118 into master will increase coverage by 0.84%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   93.38%   94.23%   +0.84%     
==========================================
  Files          14       14              
  Lines         922      971      +49     
==========================================
+ Hits          861      915      +54     
+ Misses         61       56       -5

Impacted Files	Coverage Δ
mne_bids/tests/test_io.py	100% <100%> (ø)	⬆️
mne_bids/tests/test_utils.py	100% <100%> (ø)	⬆️
mne_bids/mne_bids.py	93.67% <100%> (+0.79%)	⬆️
mne_bids/utils.py	92.73% <100%> (+1.35%)	⬆️
mne_bids/tests/test_mne_bids.py	100% <100%> (ø)	⬆️
mne_bids/io.py	89.28% <100%> (+1.28%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de54d7b...e8accaf. Read the comment docs.

[sappelhoff]

@jasmainak see here.

really simple approach, but adds some iEEG coverage and should be a good starting point for more thorough work on our iEEG parts.

[jasmainak]

Overall, I like the approach. Thanks for taking a stab at it

[jasmainak]

gain in coverage appears to be really tiny. Why is that @sappelhoff ?

[sappelhoff]

I'll do some more work tomorrow, looking at Travis CI, my fears have come true ;-) iEEG is not really implemented yet

[jasmainak]

okay if it's really a pain to fix, maybe we should do this after release ...

I think we should update the landing page of our website to indicate what modalities we support (saying iEEG is experimental) and the file formats supported (and those that aren't yet ... ?). Just a nice way to show off all the work we have put in.

[jasmainak]

@sappelhoff I'm setting the PR back to WIP since Travis is not happy.

[sappelhoff]

I think we should update the landing page of our website to indicate what modalities we support (saying iEEG is experimental) and the file formats supported (and those that aren't yet ... ?). Just a nice way to show off all the work we have put in.

+1

but regarding file formats, only MEF3 and NWB are not supported yet ... and these fall under "experimental iEEG" anyways, so I'd limit the paragraph to mentioning the modalities.

[sappelhoff]

this will not pass due to:

3: [ERR] The column names of the channels file must begin with ['name', 'type', 'units', 'sampling_frequency', 'low_cutoff', 'high_cutoff', 'notch', 'reference'] (code: 72 - MISSING_TSV_COLUMN_IEEG_CHANNELS)

see here: https://github.com/bids-standard/bids-validator/blob/cbcb0d31c5a945ef8be9e5c2b4e380b6d7a01a58/validators/tsv/tsv.js#L237-L249

Fixing this would require some change to our current channels.tsv files --> more columns.

We could add these columns and fill them with n/a until we have the time to write functions that extract the actual information.

Opinions?

[jasmainak]

Fixing this would require some change to our current channels.tsv files --> more columns.

But this would be only for iEEG or is it also for EEG and MEG?

[sappelhoff]

But this would be only for iEEG or is it also for EEG and MEG?

it would be convenient to have it for all, but I think the reason that we have this error from the validator is because @choldgraf wrote these lines quite a while ago, and they are probably subject to change soon ... so I wouldn't want to change MNE-BIDS so much just contingent on this error.

So yes, perhaps we can use it just for iEEG?

[sappelhoff]

btw, the other error

1: [ERR] Files with such naming scheme are not part of BIDS specification. This error is most commonly caused by typos in file names that make them not BIDS compatible. Please consult the specification and make sure your files are named correctly. If this is not a file naming issue (for example when including files not yet covered by the BIDS specification) you should include a ".bidsignore" file in your dataset (see https://github.com/bids-standard/bids-validator#bidsignore for details). Please note that derived (processed) data should be placed in /derivatives folder and source data (such as DICOMS or behavioural logs in proprietary formats) should be placed in the /sourcedata folder. (code: 1 - NOT_INCLUDED)

will hopefully be fixed soon, see my attempts at clarification here: bids-standard/bids-validator#510 (comment)

[jasmainak]

So yes, perhaps we can use it just for iEEG?

fine by me. But maybe if we can extract some minimal information rather than have 'n/a' that would be nice if it does not involve too many changes.

Btw, I'd prefer to get #106 merged first because it touches upon many parts of the code base and rebasing it is a pain.

[sappelhoff]

Btw, I'd prefer to get #106 merged first because it touches upon many parts of the code base and rebasing it is a pain.

+1, reviewing it right now

[jasmainak]

same here

[jasmainak]

Once these changes are also released and installable, this present PR should pass (at least all changes related to it).

amazing! sounds great to me

[jasmainak]

why is CircleCI failing?

[sappelhoff]

why is CircleCI failing?

after the merge of #129 it should be green again.

[jasmainak]

@sappelhoff can you remove the merge commit so that we have a clean history?

[sappelhoff]

@sappelhoff can you remove the merge commit so that we have a clean history?

I did, but does it matter if we use "squash and merge" anyways?

[jasmainak]

I did, but does it matter if we use "squash and merge" anyways?

I don't know. Do we have an official policy of "squash and merge" now? cc @teonbrooks ? I'm fine with anything that merges on tip of master ...

[sappelhoff]

@jasmainak this PR is ready from my side. Feel free to merge if you are happy :-)

[jasmainak]

Other than that, looks good to me.

[jasmainak]

@teonbrooks or @choldgraf merge if you are happy

[teonbrooks]

lgtm, thanks @sappelhoff!