[MRG] Simplify raw_to_bids #106

jasmainak · 2018-10-04T03:15:03Z

closes #98
closes #105
closes #119
closes #123

examples/convert_mne_sample.py

mne_bids/mne_bids.py

jasmainak · 2018-10-04T05:58:05Z

@sappelhoff @teonbrooks @monkeyman192 thoughts?

I stop for today. The diff is a bit largish but I think I got two examples to run. We should first agree that this is a more future-compatible / simpler API. Maybe it isn't ... I don't know

jasmainak · 2018-10-04T05:58:48Z

and your turn to review :-) I take a break from reviewing ...

doc/api.rst

mne_bids/mne_bids.py

sappelhoff · 2018-10-04T09:09:43Z

mne_bids/utils.py

@@ -60,6 +60,20 @@ def _mkdir_p(path, overwrite=False, verbose=False):
            raise


+def _parse_bids_filename(fname):


you need a test ;-)

typically private functions are not tested explicitly. But these lines should be covered since they are being called by the write_raw_bids function

sappelhoff · 2018-10-04T09:12:40Z

mne_bids/mne_bids.py

    # BrainVision is multifile, copy over all of them and fix pointers
    elif ext == '.vhdr':
-        copyfile_brainvision(raw_fname, raw_file_bids)
+        copyfile_brainvision(raw_fname, fname)


this will not go well together with what you write in the docstring on line 497:

The file path of the BIDS compatible raw file. In the case of multifile systems (e.g., vhdr, .ds etc.), this is the path to a folder containing all the files.

See also my comment above. Both copyfile_brainvision (a multifile system, always 3 files) and copyfile_eeglab (a facultative multifile system ... can be 1 file or 2 files) expect the location of the header file, i.e., myfile.vhdr or myfile.set ... passing a directory is currently not supported and would maybe also not be a good idea.

sappelhoff

I like the approach and the idea to make MNE-BIDS more modular! There is much to be done, but I find this approach promising and also like the idea of an analogous "read_raw_bids" in the future.

jasmainak · 2018-11-01T05:21:04Z

@agramfort : updated PR to use raw._init_kwargs. CIs should be happy once mne-tools/mne-python#5679 is in

jasmainak · 2018-11-02T17:22:02Z

@agramfort @teonbrooks CIs are happy

teonbrooks

on the whole, everything looks good and things are cleaned up.

my one concern, which is also inline in the review: I don't think that make_bids_basename should be exposed to the end user and that we should incorporate the arguments into write_raw_bids and privatize to _make_bids_basename. I think it would simplify the api, and since we greatly reduce arg overloading for each edge case with the system-specific implementation, these standard arguments (subject, run, task, and session) can live in the function happily and not overcrowded.

examples/convert_eeg_to_bids.py

teonbrooks · 2018-11-02T21:16:14Z

README.md

@@ -18,7 +18,8 @@ Next to `numpy`, `scipy`, and `matplotlib` that are included in the standard
 anaconda distribution, you will need to install the following dependencies
 to be able to use `mne_bids`:

-    $ pip install pandas mne
+    $ pip install pandas
+    $ pip install -U https://api.github.com/repos/mne-tools/mne-python/zipball/master


maybe we can note the mne-python current version as the minimal in the text above

not sure what you mean?

I guess we don't specify anywhere minimal version number. like, we should have a section that says mne >= 0.17.X

That's probably because we don't know them. I know only for MNE we need >=0.17.x. For the rest I would say the "latest" :-)

examples/convert_eeg_to_bids.py

mne_bids/commands/mne_bids_raw_to_bids.py

mne_bids/io.py

mne_bids/pick.py

jasmainak · 2018-11-02T21:52:24Z

I don't think that make_bids_basename should be exposed to the end user and that we should incorporate the arguments into write_raw_bids and privatize to _make_bids_basename

So, we have an example for this function here. Do you envision this example going away if it becomes private?

I would just let the user create the BIDS filename using string formatting but if you think that's adding friction, we can also have it as function arguments. No strong opinion there. My only concern is that since BIDS is still evolving, it might make sense not to tie API decisions to the file naming structure. There are still key-value pairs being added and dropped ...

teonbrooks · 2018-11-02T22:01:31Z

There are still key-value pairs being added and dropped ...

grr... yeah, does this include all current applicable key-value pairs? if they are exposed within the function as optional, then I would actually be ok with having this as a separate function.

and yes, i do realize i was the one who suggested the basename function and then critique it afterwards lol ;)

jasmainak · 2018-11-02T22:07:49Z

grr... yeah, does this include all current applicable key-value pairs?

yep exactly. You would need to give people a way to write filenames with all the key-value pairs in BIDS, not just a subset ...

mne_bids/utils.py

examples/convert_eeg_to_bids.py

mne_bids/mne_bids.py

agramfort · 2018-11-03T12:30:42Z

in make_bids_folders you use root for what you call output_path elsewhere no?

jasmainak · 2018-11-03T20:44:22Z

Good catch. Now I made the variable output_path consistently everywhere

teonbrooks · 2018-11-05T01:51:59Z

everything looks good here now and all tests passing. merging.
thanks heaps @jasmainak!

jasmainak force-pushed the refactor_raw_to_bids branch 17 times, most recently from 4564c92 to 32b2cf8 Compare October 4, 2018 05:51

jasmainak commented Oct 4, 2018

View reviewed changes

examples/convert_mne_sample.py Outdated Show resolved Hide resolved

jasmainak commented Oct 4, 2018

View reviewed changes

mne_bids/mne_bids.py Outdated Show resolved Hide resolved

monkeyman192 reviewed Oct 4, 2018

View reviewed changes

mne_bids/mne_bids.py Outdated Show resolved Hide resolved

jasmainak commented Oct 4, 2018

View reviewed changes

doc/api.rst Outdated Show resolved Hide resolved

sappelhoff reviewed Oct 4, 2018

View reviewed changes

mne_bids/mne_bids.py Outdated Show resolved Hide resolved

sappelhoff reviewed Oct 4, 2018

View reviewed changes

mne_bids/mne_bids.py Outdated Show resolved Hide resolved

sappelhoff reviewed Oct 4, 2018

View reviewed changes

sappelhoff requested changes Oct 4, 2018

View reviewed changes

jasmainak mentioned this pull request Oct 11, 2018

MRG: Make kind=auto #105

Closed

jasmainak force-pushed the refactor_raw_to_bids branch from 4146d43 to 4307cbe Compare October 14, 2018 21:52

jasmainak mentioned this pull request Oct 31, 2018

[BUG] _channels_tsv writes default unit for channel type #81

Closed

ENH: Use raw._init_kwargs to check raw.times

a471299

sappelhoff mentioned this pull request Nov 1, 2018

[WIP] fix channel units for EEG and iEEG #124

Closed

DOC: update readme

30df0b4

sappelhoff mentioned this pull request Nov 2, 2018

[MRG] fix channel units for EEG and iEEG #125

Merged

TST make travis and appveyor happy

21c6052

teonbrooks reviewed Nov 2, 2018

View reviewed changes

mne_bids/utils.py Show resolved Hide resolved

teonbrooks reviewed Nov 2, 2018

View reviewed changes

examples/convert_eeg_to_bids.py Outdated Show resolved Hide resolved

teonbrooks reviewed Nov 2, 2018

View reviewed changes

mne_bids/mne_bids.py Outdated Show resolved Hide resolved

teonbrooks reviewed Nov 2, 2018

View reviewed changes

mne_bids/mne_bids.py Outdated Show resolved Hide resolved

teonbrooks reviewed Nov 2, 2018

View reviewed changes

mne_bids/mne_bids.py Outdated Show resolved Hide resolved

jasmainak force-pushed the refactor_raw_to_bids branch from dacba65 to d9ea3a7 Compare November 3, 2018 01:06

FIX: address comments by Teon

665fbf5

jasmainak force-pushed the refactor_raw_to_bids branch 3 times, most recently from d4d0ac7 to a9674c3 Compare November 3, 2018 01:35

bids_fname -> bids_basename

6e5beea

jasmainak force-pushed the refactor_raw_to_bids branch from a9674c3 to 6e5beea Compare November 3, 2018 01:37

misc

4c378c5

root -> output_path and _age_on_date private

75b0cb6

teonbrooks merged commit c79dc4a into mne-tools:master Nov 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Simplify raw_to_bids #106

[MRG] Simplify raw_to_bids #106

jasmainak commented Oct 4, 2018 •

edited by teonbrooks

Loading

jasmainak commented Oct 4, 2018

jasmainak commented Oct 4, 2018

sappelhoff Oct 4, 2018

jasmainak Oct 15, 2018

sappelhoff Oct 4, 2018

sappelhoff left a comment

jasmainak commented Nov 1, 2018

jasmainak commented Nov 2, 2018

teonbrooks left a comment •

edited

Loading

teonbrooks Nov 2, 2018

jasmainak Nov 2, 2018

teonbrooks Nov 2, 2018

jasmainak Nov 2, 2018

jasmainak commented Nov 2, 2018

teonbrooks commented Nov 2, 2018

jasmainak commented Nov 2, 2018

agramfort commented Nov 3, 2018

jasmainak commented Nov 3, 2018

teonbrooks commented Nov 5, 2018

		@@ -60,6 +60,20 @@ def _mkdir_p(path, overwrite=False, verbose=False):
		raise


		def _parse_bids_filename(fname):

[MRG] Simplify raw_to_bids #106

[MRG] Simplify raw_to_bids #106

Conversation

jasmainak commented Oct 4, 2018 • edited by teonbrooks Loading

jasmainak commented Oct 4, 2018

jasmainak commented Oct 4, 2018

sappelhoff Oct 4, 2018

Choose a reason for hiding this comment

jasmainak Oct 15, 2018

Choose a reason for hiding this comment

sappelhoff Oct 4, 2018

Choose a reason for hiding this comment

sappelhoff left a comment

Choose a reason for hiding this comment

jasmainak commented Nov 1, 2018

jasmainak commented Nov 2, 2018

teonbrooks left a comment • edited Loading

Choose a reason for hiding this comment

teonbrooks Nov 2, 2018

Choose a reason for hiding this comment

jasmainak Nov 2, 2018

Choose a reason for hiding this comment

teonbrooks Nov 2, 2018

Choose a reason for hiding this comment

jasmainak Nov 2, 2018

Choose a reason for hiding this comment

jasmainak commented Nov 2, 2018

teonbrooks commented Nov 2, 2018

jasmainak commented Nov 2, 2018

agramfort commented Nov 3, 2018

jasmainak commented Nov 3, 2018

teonbrooks commented Nov 5, 2018

jasmainak commented Oct 4, 2018 •

edited by teonbrooks

Loading

teonbrooks left a comment •

edited

Loading