-
Notifications
You must be signed in to change notification settings - Fork 85
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
[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.
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should eventually just change it to ext in blah
. This is so confusing ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only ext now that should be in that list is .pdf
right? Just wanting to finish off this PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so ...
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: