[MRG] KIT data now goes in the meg folder
#187
Conversation
Codecov Report
@@ Coverage Diff @@
## master #187 +/- ##
==========================================
- Coverage 95.48% 95.46% -0.02%
==========================================
Files 17 17
Lines 1218 1214 -4
==========================================
- Hits 1163 1159 -4
Misses 55 55
Continue to review full report at Codecov.
|
monkeyman192
changed the title
meg foldermeg folder
|
how is the CI passing? is the validator already updated? and if so, how was it passing before? |
|
I think the validator can handle having raw files in a sub-folder or not. I don't think it was coded to require KIT data to be specifically in a sub-folder (haven't checked the code though, just assuming because it does pass...) |
I think so as well ... the validator is not checking thoroughly in some parts ... and for other parts it's annoyingly accurate ... |
|
right, but then all the discussion is a waste since nothing forces them to stick one way or another ... but since we have round trip I/O now, we don't rely on validator that much for the tests. So, I'm okay merging this. |
| @@ -685,7 +685,8 @@ def write_raw_bids(raw, bids_basename, output_path, events_data=None, | |||
| channels_fname = make_bids_basename( | |||
| subject=subject_id, session=session_id, task=task, run=run, | |||
| acquisition=acquisition, suffix='channels.tsv', prefix=data_path) | |||
| if ext not in ['.fif', '.ds', '.vhdr', '.edf', '.bdf', '.set']: | |||
| if ext not in ['.fif', '.ds', '.vhdr', '.edf', '.bdf', '.set', '.con', | |||
| '.sqd']: | |||
There was a problem hiding this comment.
we should eventually just change it to ext in blah. This is so confusing ...
There was a problem hiding this comment.
the only ext now that should be in that list is .pdf right? Just wanting to finish off this PR...
yes, we should make an issue on the validator :-) (go for it!) |
|
What's the status on this issue @jasmainak @monkeyman192 ? |
|
We need to:
|
|
@jasmainak @sappelhoff, we should get this included in the release. |
|
@sappelhoff maybe you can push to this pr so we can merge it
On Fri 26 Apr 2019 at 08:13, Teon L Brooks ***@***.***> wrote:
@jasmainak <https://github.com/jasmainak> @sappelhoff
<https://github.com/sappelhoff>, we should get this included in the
release.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#187 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADY6FIRDCEDHPFBRZFQ5SWDPSKMRPANCNFSM4HCFHQOQ>
.
--
Sent from my iPhone
|
|
I'll include it in #196 but @jasmainak --> what do you mean by "need to update the conditional"? In your comment |
PR Description
closes #186
Merge checklist
Maintainer, please confirm the following before merging: