mne-tools / mne-bids

MNE-BIDS is a Python package that allows you to read and write BIDS-compatible datasets with the help of MNE-Python.
https://mne.tools/mne-bids/
BSD 3-Clause "New" or "Revised" License
132 stars 86 forks source link

BrainVision export for MNE #41

Closed palday closed 5 years ago

palday commented 6 years ago

This isn't really an issue to solve but rather an announcement to save duplication of effort. It looks like BrainVision and EDF will be the preferred formats for BIDS-EEG, so it's important that MNE-BIDS has a way of writing to those formats.

I recently implemented a basic BrainVision writer for MNE-Python Raw objects as part my own package of convenience functions. I have plans to extend support to reading and writing MNE-Python Epochs objects.

Following discussions with @robertoostenveld, the core parts of this code will be re-factored and moved to a separate package (planned name: pybv) and will not be dependent on MNE in any way, but rather manipulate simple NumPy arrays for the data and events and a dictionary of attributes. The MNE-based writer functionality will then be reworked to simply be wrappers around this functionality.

Similarly, @robertoostenveld has been kind enough to license his pure-python EDF reader and writer under the 3-clause BSD license. I'll also be packaging that up in the next few weeks and putting that on PyPI as pyEDF. There are already two existing EDF packages available on PyPI; however, they are both dependent on the EDFlib C/C++ library.

After the refactoring, repackaging and adding a good CI test suite, the plan is to offer to transfer ownership of the repositories to INCF. The code will be intentionally minimal in terms of reading and writing arrays and not depending on MNE. That said, there are some rather nice array-based constructors for the MNE object classes, so it should be straightforward to e.g. integrate convenience functions for the writers under an mne.export module and potentially refactor the current readers to use these external libraries. But that's a bridge the broader MNE community can decide whether to cross when we get there.

Again, this is mostly an announcement in a convenient forum because a lot of this discussion has happened in private emails or in person and thus isn't accessible to the MNE-Python/BIDS community.

@choldgraf @sappelhoff This isn't quite just giving the writer over to MNE-BIDS, but it should still help.

agramfort commented 6 years ago

cool. Keep us informed about your progress.

thanks for sharing.

choldgraf commented 6 years ago

+1 to having INCF own the guts of the conversion, and using a lightweight wrapper around this in MNE-BIDS

jasmainak commented 6 years ago

sounds good !

sappelhoff commented 6 years ago

+1 I couldn't have wished for more! :-)

for pybv it would be cool to have a renaming utility such as this one. I have seen that in philistine you had already reserved a function for that so I guess it's part of the plan anyhow.

choldgraf commented 6 years ago

@palday any idea on when you think pybv will start moving forward? It's looking like BIDS-iEEG and BIDS-EEG will support brainvision and EDF as two first-class citizens, so it'd be great to have some Python export functionality for these formats that is well-tested/documented/etc

palday commented 6 years ago

Not this week, my day job has some more pressing demands ... The export functionality for MNE Raw already exists in philistine (on PyPI!) and is well documented and has a full test-suite based on synthetic data. I'm still fine-tuning some of the support for the different binary formats, but that can occur after migrating to pybv. Refactoring (to remove the MNE dependency) is part of the move to a separate package, so it'll take more than just a few minutes to copy the source and create a new setup.py.

teonbrooks commented 6 years ago

random thought, an ephys equivalent to nibabel.............babelphys

agramfort commented 6 years ago

python-neo : https://github.com/NeuralEnsemble/python-neo

had this vision before you :)

choldgraf commented 6 years ago

@agramfort have you used python-neo before? I've never given it a shot. Does it have strong development?

agramfort commented 6 years ago

it's active but has a smaller community than nibabel

we use it in mne for certain IOs

we could make a much tighter integration but we need the ressources for this.

bdyetton commented 6 years ago

Just adding my +1 for .edf saving in MNE.

agramfort commented 6 years ago

maybe someone can do a POC with https://github.com/robertoostenveld/pyEDF ?

palday commented 6 years ago

I'm working on it....

bdyetton commented 6 years ago

@palday Great! I have a collection of EDF's with different channels, annotations, versions, etc (there is edf+ and edf++ apparently). I'm happy to test out whatever you come up with on these.

palday commented 6 years ago

@bdyetton Cool, that's something for the future at the moment -- right now, we're focusing on vanilla EDF. Extensions (EDF+ andEDF++; BDF and GDF) may one day be supported, but I have a rather large amount of code I owe various people before I get around to that ....

sappelhoff commented 6 years ago

@palday do you have a timeline for the implementation of the BrainVision writer? We are getting closer and closer to the finish line with the BIDS specification for EEG, and while I have seen some progress on https://github.com/robertoostenveld/pyEDF, there is no pyBV or pyBrainVision yet.

Just asking so that potentially some of us could already get started on the pure python (i.e., no MNE) BrainVision reader/writer if you are swamped with other work.

choldgraf commented 5 years ago

another friendly ping - BIDS-iEEG is going to be merged pretty soon, but we still don't have any well-supported brainvision or EDF export (or somebody correct me if that is wrong!)

I'm a bit worried that we're going to start releasing specifications as "official" without having the ability to actually write these to disk from other formats :-/

jasmainak commented 5 years ago

Currently, in MNE-BIDS we are able to convert EDF and brainvision to BIDS format. The way we do it to copy the original files over and extract the metadata. Since BIDS is meant to be for unprocessed data, that should do the job, no?

choldgraf commented 5 years ago

In my case the data is in FIF, but it's for iEEG data which doesn't support FIF. That means I can't just copy/paste it over :-/

jasmainak commented 5 years ago

In my case the data is in FIF

are you talking about native data or processed data that is in fif? I thought fif was unique to Elekta, so how come you have iEEG data in fif?

choldgraf commented 5 years ago

I have iEEG data in every format under the sun haha, but my workflow involved first converting that data into .fif and storing it as "as close as possible to raw" format.

jasmainak commented 5 years ago

okay I see. Looks like you guys really needed a standard ;-)

choldgraf commented 5 years ago

lol ieeg is in much worse shape than meg/eeg, and that's saying something :-)

sappelhoff commented 5 years ago

If @palday is okay with it, we could start a brainvision writer from the "bids-standard" organization github account.

If some of us work together we can have a decent writer up in no time. What do you think, @choldgraf? Perhaps @jasmainak wants to come on board, we'd probably need some of the never-ending reviews one can have with him ;-)

palday commented 5 years ago

The MNE to BV writer already exists in philistine .... since it mostly just treats the Raw object as a container for an NumPy array and some metadata doesn't really depend on any of its associated methods, it shouldn't be too hard to make it for generic array-likes. It doesn't support all combinations of binary format and orientation nor does it currently support segments (mostly because the latter isn't relevant for the data I work with for my data job, so I don't have good examples), but these are less pressing features at the moment, I think.

That said, what's really missing is a more extensive test suite on real data. I can "move" (copy & paste) the writer to pybv. What would be most helpful for me would be creation of a more extensive test suite, including the ability to download test data from another location so that it's not all stored in the package itself (i.e. do it the way it's done in MNE). Currently, synthetic data is used to avoid this, but it would be nice to have real data examples.

Otherwise, pybv has the code to process VHDR and VMRK as the deviant forms of INI that they are.

Finally, if it's really just about having a Python implementation of the writer: PyCorder is GPL'd and does record data in BV format ....

sappelhoff commented 5 years ago

It doesn't support all combinations of binary format and orientation nor does it currently support segments (mostly because the latter isn't relevant for the data I work with for my data job, so I don't have good examples), but these are less pressing features at the moment, I think.

I agree that advanced features are not pressing at the moment. We can build them over time.

I can "move" (copy & paste) the writer to pybv

I think that this would be a very good step. And I would suggest moving it to a "pybv" repository owned by @bids-standard. In my opinion it would also be great to move https://github.com/robertoostenveld/pyEDF from @robertoostenveld to @bids-standard ... what are your opinions on this? @palday suggested to transfer the ownership to @INCF in the original post, which would also be fine by me.

What would be most helpful for me would be creation of a more extensive test suite, including the ability to download test data from another location so that it's not all stored in the package itself

Great, I'd be happy to get started on a PR for that as soon as we have figured out the details above.

pybv has the code to process VHDR and VMRK as the deviant forms of INI that they are.

In the meantime, we have received an official documentation of the BrainVision format by BrainProducts themselves. We can use that to even out potential differences and add potentially missing fields

if it's really just about having a Python implementation of the writer: PyCorder is GPL'd and does record data in BV format ....

The PyCorder Software is not as open as I would like it to be: Apparently you have to first register in some kind of forum, which requires you to have purchased an actiChamp (you need a serial number at least), see the screenshot:

image

... but it'd certainly be a good source to look into for more inspiration.

palday commented 5 years ago

PyCorder is GPL'd, which means that once anybody has access, they can distribute it e.g..

palday commented 5 years ago

And for the love of all that is holy and still a little bit private, please don't share things via Google Docs.

choldgraf commented 5 years ago

My 2 cents is that we should focus first on minimum viable functionality of an exporter (meaning: basic I/O, good documentation, and a test suite) before worrying about fancier features. I'm super hesitant to "recommend" that users try a piece of software if it doesn't meet the above points, so I think that's where we should start.

I'll give philistine another go!

teonbrooks commented 5 years ago

I spoke with BrainProducts at SfN and they are really keen to help us with all things BIDS related and they asked us how they could be serve our needs. i have a line of contact with their lead for strategic product management.

@sappelhoff thanks for the pdf share. do you know if they have this hosted on their site or somewhere. it would be good to have an archive for their different version for max compatibility. @palday what was the apprehension with gdocs?

I concur think having pybv as part of bids-standard is a good move. I think we can take it as it is, and then we could make less reliant on mne Raw so that it is package-agnostic but I think this is a good starting ground.

sappelhoff commented 5 years ago

@teonbrooks Yes, Brain Products will certainly share it on their own web page (they are happy of course that BIDS adopts the BrainVision format), however this is going to take some more time and perhaps meetings to finalize the specification I shared. The document I shared is "work in progress" but it was provided to us to share it on BIDS channels, which we have done so far in the fieldtrip wiki.

regarding the gdocs, the file is actually stored on github - I just use the gdocs pdf viewer to conveniently display it in a browser using https://docs.google.com/viewer?url=..... (replace .... ). Having said that, I just realized that Github also displays PDFs: cool.

palday commented 5 years ago

(I have an even bigger rant for colleagues who believe that Dropbox is a good way to collaborate.)

choldgraf commented 5 years ago

hey all - I am pretty much in agreement with everything that @palday says re: google, but a gentle request that we keep this issue focused on EDF/BrainVision exporting :-)

teonbrooks commented 5 years ago

@choldgraf @dorahermes are there things special to note about using BV format for ieeg? If it is straightforward, I think we should start with pybv. the path to making it work with mne-bids seems straightforward and would work as a testing ground for compatibility for other packages.

dorahermes commented 5 years ago

I have been exporting my iEEG data in BV using Fieldtrip tools (all data that I work with are starting to get BIDSified) would be great if there is an MNE Python equivalent.

jasmainak commented 5 years ago

@dorahermes can you share some scripts which you have been using? As I said above, if you do not convert your data, mne-bids should already be able to handle BIDSifying for BV. For me, BIDS-iEEG should support formats that can be easily read and are in original manufacturers format. Converting between file formats is likely to lead to loss of metadata / information.

jasmainak commented 5 years ago

@choldgraf @dorahermes thinking a bit more about this.

If users are obliged to convert their data to brainvision / edf in addition to extracting the metadata, you are introducing friction in adapting the BIDS format. It is now no longer a matter of renaming files and putting useful information, but also making sure your data is the same after the conversion.

If on the other hand, the idea is to promote a single file format, I think there shouldn't be 4 other options. There should be stronger recommendations for a single file format. By recommending 5 formats, it's now 10x more work (input + output * 5) for developers and 2x more work for users ...

sappelhoff commented 5 years ago

Not related to Mainak's comment: we now have a pybv repository: https://github.com/bids-standard/pybv

dorahermes commented 5 years ago

I added an example script in the bids-starter-kit: https://github.com/bids-standard/bids-starter-kit/blob/master/matlabCode/createBIDS_ieegdata_WriteBrainVisionWithFieldtrip.m

We have had an extensive community discussion about the data format:

The following formats will not be supported yet, but are allowed. These needed to be there for several reasons:

palday commented 5 years ago

@sappelhoff It might have made sense to fork the existing one: https://github.com/palday/pybv . Someone with write access and of course just pull from mine and push to the empty one if so desired.

jasmainak commented 5 years ago

@dorahermes thanks for the detailed explanation! And sorry if I'm making you repeat old discussion. It might be a good idea to document these somewhere.

What's the minimum possible that could be done to support 80% of use cases for iEEG in MNE? Would you say that brainvision writer will do the job? If so, let us rename this issue to exclude EDF. I don't want to be in a situation where we have to maintain 3 or 4 more file formats. It's not sustainable for a small team like ours ...

choldgraf commented 5 years ago

a brainvision writer that converts from FIF will be good enough, I think. So if we can get philistine (or pybv) working then I think we're in good-enough shape. We can then just document the your format -> mne RawArray -> vhdr workflow

jasmainak commented 5 years ago

okay changed issue title to make the discussion more focused. For tests, if you are using MNE, you can check if round trip works. We have brainvision files on our repo.

sappelhoff commented 5 years ago

@sappelhoff It might have made sense to fork the existing one: https://github.com/palday/pybv . Someone with write access and of course just pull from mine and push to the empty one if so desired.

Yes, let's do that. However in https://github.com/palday/pybv I don't see the code that you have in philistine (see io) ... instead, there is a new "work in progress"-like file: vhdr_as_config

We would also have to change a few things such as the setup.py. Would you like to stay in charge as the maintainer @palday? We can certainly give you write access to bids-standard/pybv

dorahermes commented 5 years ago

brainvision writer only would be great, thanks!

@jasmainak there will be an extensive paragraph on this topic in the paper, which we are starting to wrap up

palday commented 5 years ago

@choldgraf As far as I know, the writer in philistine works. It passes all the existing tests ....

@sappelhoff Of course the philistine code isn't in there -- it's still dependent on MNE and the whole point of pybv (as announced above) was to have an independent implementation that MNE could optionally take advantage of for both reading and writing. I'm amenable to contributing the philistine function to MNE as the first function in a new export submodule -- I don't think it makes sense to have the writers for arbitrary formats be part of the io module because they are all export operations and potentially somewhat lossy (too much of MNE's internal representation is based on FIFF). When pyEDF is far enough along (there are some issues in the preliminary tests I've run), I would be amenable to contributing to export a wrapper function that takes an MNE Raw object and extracts the bits and pieces that pyEDF needs.

I hadn't yet handed over ownership of pybv because it wasn't in a coherent form yet. I really want to take advantage of the INI structure in VMRK and VHDR, and that's what's currently in pybv -- the reading of VHDR as an INI file via configparser. I have several EDF test files that @robertoostenveld contributed and could use help setting up an MNE-like data fetching routine for testing and examples. I'm hesitant to depend on MNE's data repository because the whole point was to not depend on MNE and I haven't checked the license(s) for distributing said data outside of MNE. I also have several BrainVision test files recorded at different resolutions and sample rates with a test signal. The BrainVision recordings are new and my own and don't involve any participants, so I can upload them to OSF or some other good hosting site.

@jasmainak Maybe the title was completely wrong and should have been "Independent, pure Python implementation of BrainVision and EDF readers and writers are under development that MNE can potentially one day take advantage of".

agramfort commented 5 years ago

Yes put it outside of mne and maybe in the future mne can rely on this as an external package. Just see how to reuse as much as possible our test files

sappelhoff commented 5 years ago

For future reference, in the GPL PyCorder software by BrainProducts, the relevant lines for writing to BrainVision format are starting around here: https://github.com/EIT-team/PyCorder/blob/e4c236cc787b7db254a4175c814bfde0f598b42b/storage.py#L391-L420

sappelhoff commented 5 years ago

Lastly, for BrainVision testing, I found three datasets on Zenodo that might be helpful if we want to work on non-synthetic data or data that does contain subjects, see the list by license:

CC0 1.0

CC BY 4.0

choldgraf commented 5 years ago

hey all - I took a look through @palday 's philistine package, and think that it looks like a really nice foundation for an MNE-agnostic brainvision export package. Since there's now an (empty) pybv repository in the bids-standard, I propose doing this:

  1. Someone (maybe me?) adapt the philistine package to the bids-standard/pybv repo so that it is MNE-agnostic (e.g. any time that it extracts info from a "raw" object, just make those explicit parameters in the function and expect numpy arrays for the data)
  2. Treat this as an "alpha-stage" MVP for pybv, note that the API and internals may change
  3. Start iterating off of this as a first-step for pybv.

What do folks think about that?