Add functionality to export to EEGLAB .set

[jackz314]

Reference issue

N/A

What does this implement/fix?

This adds the ability to export Raw or Epochs instances as EEGLAB set files. As well as the ability to import channel location data from separate CSV files or another file (using read_raw).

Additional information

The exported to set function (save_set) was based on mnelab's write_set. I added channel location export and events/epochs data export so that Epochs exports can be read by EEGLAB.

The channel location expansion function (xyz to all, used for save_set) referenced MATLAB's cart2sph, as well as EEGLAB's relevant functions.

It has been roughly tested on a few data sets on both EEGLAB and MNE.

[jackz314]

Seems like the tests are failing due to the lack of docstrings. Should I add them?

[mmagnuski]

Welcome on your first mne PR, @jackz314!
Thanks for contributing, beinging able to save files in .set format would be a nice addition (at least from my perspective).

Seems like the tests are failing due to the lack of docstrings. Should I add them?

Yes, we follow the numpy docstring format. Take a look around in the codebase - you will see the pattern. The docstrings are required for all public functions.
Adding a functionality like that also requires writing tests that cover > 90% of the added code.
Also - make sure you follow the PEP8 code style.
You will find more guidelines here.

[agramfort]

thx @jackz314 for the work

we should discuss how to approach this. So far we have been reluctant to support writing for something different from .fif as it's very likely that the save to something else will not persist to disk all the informations in the mne instance.

Also you add a lot of public functions to mne and we are support careful about this. Should i had started this PR I would have tried to support writing without any new public function: just use .save method and detect the file extension to know what to do.

[hoechenberger]

Nice job, @jackz314!
Like @agramfort said, ideally all functions except for save_set() should be private.

Also I'm wondering if instead of adding save_set(), we should consider adding a parameter to save(), allowing to save in EEGLAB format. Or it could even auto-pick "EEGLAB .set" if the output file extension is .set.
But this really needs a bit of discussion (and, most certainly, much more testing…)

[jackz314]

Thanks guys. I'm currently writing tests on this, but my expertise in both EEGLAB and MNE is limited, so I would probably need help from other people to test this more thoroughly - As I'm writing these tests, I'm already discovering multiple bugs in my code. I agree maybe merging save_set with save would be better.

The only two user-facing functions would probably be save_set and import_eeg_chan_location_from_csv. get_eeglab_full_cords and cart_to_eeglab_full_coords doesn't seem to be useful outside of conversion between eeglab and mne, so I can change them to private.

As for information loss, unfortunately, it seems like there will be some unavoidable info loss, so far from my limited testing, things like re-reference, baseline, and high/low pass freq info are lost when converting between the two. For re-referencing and hi/lo pass, the modified data is preserved, but in EEGLAB there's no way of knowing what kind of re-referencing is done, and when converting back to MNE, there's no way of getting the filtering parameters.

Info lost is definitely not ideal, but in a lot of cases it's tolerable (as long as data is intact and as expected), so I think maybe some warning could be given to users who try to export as set files, that way they are aware of the info lost, but can still benefit from the conversion. I'm currently working with a lab where EEGLAB is heavily used, but they also want to use Python as well, since some things like ML is easier on Python, this is why I wrote these functions, and I think other people in similar situations would benefit from this as well.

[hoechenberger]

import_eeg_chan_location_from_csv

This one should probably also be integrated into read_custom_montage()

[jackz314]

import_eeg_chan_location_from_csv

This one should probably also be integrated into read_custom_montage()

I just realized that channel location data in MNE is called Montage, I will integrate it into read_custom_montage(), but it should probably be a separate PR.

[hoechenberger]

I will integrate it into read_custom_montage(), but it should probably be a separate PR.

This actually sounds like a good idea!!

[jackz314]

I also would like to know where I should put the tests for these functions. Currently, I put the tests for Epochs' save set to tests/test_epochs.py, and I plan on putting the tests for Raw's save set to io/tests/test_raw.py, would this be appropriate? Or should I put both of them into io/eeglab/tests/test_eeglab.py since they are related to EEGLAB?

[jackz314]

I noticed that channel location data is actually stored in the format of -y, x, z instead of x, y, z. Is there a particular reason for this? Additionally, is this a standard that applies to all location data or specific to some formats like EEGLAB set files?

[hoechenberger]

Hello @jackz314,

I briefly talked with @agramfort about this effort and we had the feeling that adding support for writing to new formats could end up as opening Pandora's box: it would be very challenging to ensure that no data is lost (or even to quantify as to which data is lost, specifically) during an I/O roundtrip.

We felt that maybe this functionality would be better suited in an external package, which could very well be maintained under the MNE umbrella (mne-tools GitHub organization), but could be released separately from MNE, which would make a lot of sense as this would allow to push out bugfixes on an independent schedule.

Another idea I had and which is kind of contradictory to what I said above is adding a new method for exports that possibly lead to data loss. This is essentially what GIMP does: "save/save as" saves to their native format; "export" converts the data, possibly losing information. So I would vote for calling a method that exports to non-FIFF files export().

I'd love to hear your and the other developers' thoughts!

[jackz314]

Personally, I think this feature would be helpful even if there is some data loss, like I mentioned above, I think it would be better to provide warnings to users who use this function (and maybe on relevant docs as well) about potential data loss than to not provide the feature at all. As you said, I think using a separate and distinct name like export instead of save or save_set could further make sure that the user knows what they are doing.

Some data will be lost unfortunately due to some features being implemented very differently in EEGLAB and MNE (e.g., reference channel/method, filtering configuration, etc.). However, since the core data and some other fields like events/trials/epochs/annotations (the events "family") and channel info can mostly be exchanged without data loss, exporting will still be helpful even if other fields might not survive the i/o cycle.

I'm leaning more towards your second idea, because I feel like since other functions that read from EEGLAB are already maintained in the main MNE package, any changes to EEGLAB that requires any updates to MNE's I/O should occur naturally together. I also think that maintaining a separate package just for exporting to EEGLAB seems a bit overkill and shouldn't be necessary, especially since reading from EEGLAB is already in MNE. Still, I'm also fine with starting a new package, if the consensus later agrees that this feature should be in a separate package, I'd be happy to port my code over.

[hoechenberger]

Pinging @larsoner, @jasmainak, @adam2392, @sappelhoff, and @cbrnr

[agramfort]

i hear you Jack but it's a non-trivial maintenance cost and I anticipate a lot of complaints because some data was lost during IO round trip and user "did not read the warning". it also means a lot of CI time for testing as suddenly we need to duplicate many IO tests to guarantee nothing breaks when people save to .set files.

[adam2392]

Don't we have a "pybv" package for brainvision? Perhaps we have:

• Pyedf (I think there's pyedflib but writing was non trivial)
• Pyeeglab?

Maybe "raw.export" just calls these underlying packages? Unit testing should primarily be in those sub packages to address @agramfort objection?

the only reason I even use eeglab is cuz their ICA is more advanced then mne. Don't use them for anything else :p. Imo these kinds of write/export functionality will be very attractive for cross-platform usage. Could even have things like:

Pypersyst
Pynihonkohden

? This can be useful for mne-bids too for writing other supported BIDS formats of EEG/IEEG.

[agramfort]

+1 for a pyeeglab package now regarding the export I am not convinced

[hoechenberger]

@adam2392

Maybe "raw.export" just calls these underlying packages? Unit testing should primarily be in those sub packages to address @agramfort objection?

Yeah that sounds like a possibility. Raw.export() would just be a few lines then, passing self to one of those packages and emitting a warning – that's about it.

the only reason I even use eeglab is cuz their ICA is more advanced then mne.

It's off-topic alright, but could you elaborate? Do you mean the interactive exploration of components..?

[adam2392]

They have this nice feature which plugs into a database of features I think that can automatically estimate ICA labels in case you have 100s of subjects and can't feasibly run manual ICA, they actually have a cool feature that will match it against known labels to try to estimate if it's eye blink, heart beat, etc.

Sorry off topic yeah :p.

[hoechenberger]

They have this nice feature which plugs into a database of features I think that can automatically estimate ICA labels in case you have 100s of subjects and can't feasibly run manual ICA, they actually have a cool feature that will match it against known labels to try to estimate if it's eye blink, heart beat, etc.

Sorry off topic yeah :p.

Ah, that one. Yes we should add this to MNE too. Actually shouldn't be too difficult, I believe! We already do have a pattern matching infrastructure.

[hoechenberger]

Two more nitpicks and then I'm happy 😃

[hoechenberger]

I think we only want to print the keys here, separated by commas, i.e. something like

', '.join(supported_formats.keys()

[jackz314]

Well someone could attempt to use file extensions to infer automatically, and if the extension is not supported I think printing out the dictionary might be more helpful compared to just the formats.

[jackz314]

I can also print the dictionary in a better way (e.g. eeglab (.set), brainvision (.eeg, .vmrk, ...))

[hoechenberger]

Great idea!

[hoechenberger]

👌👌👌👌👌👌👌❤️

[hoechenberger]

@cbrnr Feel free to merge if you're happy!

Fantastic work, and thank you so much for your endless patience, @jackz314!

[agramfort]

Suggested change

fmt : 'auto' | 'eeglab'

[agramfort]

Suggested change

fmt : 'auto' | 'eeglab'

[cbrnr]

@hoechenberger can you check what needs to be done for @agramfort's latest comments? I think we're not yet ready.

[jackz314]

@hoechenberger can you check what needs to be done for @agramfort's latest comments? I think we're not yet ready.

Yeah I removed the redundant code but I'm not sure about the docstring part.

[larsoner]

Code looks good, just minor suggestions really!

However, I don't think we should merge until this is solved, i.e., eeglabio makes a PyPi release:

$ pip install --user eeglabio
ERROR: Could not find a version that satisfies the requirement eeglabio
ERROR: No matching distribution found for eeglabio

and then we add eeglabio to github_actions_dependencies.sh in the same place as nitime requirements_testing_extra.txt. I suspect that none of these lines are really being tested by CIs, and codecov agrees:

codecov/patch — 22.32% of diff hit (target 95.00%)

I know that there is already a pip install -i https://test.pypi.org/simple/ eeglabio in the eeglabio instructions, but I think users will rightly assume they should be able to pip install eeglabio, and we should wait until that's available. Also you might consider setting up a conda-forge recipe for it, maybe @hoechenberger can help with that end?

[larsoner]

@jackz314 now that #9337 is merged, if you rebase or merge with upstream/main you should be able to add eeglabio to requirements_testing_extra.txt once you make a PyPi release, and it should make CIs actually test the new functionality

[jackz314]

@jackz314 now that #9337 is merged, if you rebase or merge with upstream/main you should be able to add eeglabio to requirements_testing_extra.txt once you make a PyPi release, and it should make CIs actually test the new functionality

I moved eeglabio from Test PyPI to PyPI and added it as an requirement. The tests should now run on CIs as well.

[agramfort]

you have some remaining comments from @larsoner to address. thx !

…

[jackz314]

you have some remaining comments from @larsoner to address. thx !
…

For some reason, I didn't see the comments before. I just addressed them.

[larsoner]

Thanks for this @jackz314 !