Remove MNE export

[cbrnr]

MNE export should be handled by MNE, so I'm removing related functions and tests (and will be including them in MNE in a separate PR). Fixes #119.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e1a6c79) 100.00% compared to head (757dbdf) 100.00%.
Report is 4 commits behind head on main.

❗ Current head 757dbdf differs from pull request most recent head 982794b. Consider uploading reports for the commit 982794b to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #120   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         3    -2     
  Lines          634       541   -93     
=========================================
- Hits           634       541   -93     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sappelhoff]

sorry for apparently being misleading in my earlier comment #119 (comment) ... the idea was originally (together with e.g., @larsoner) that all MNE-Python "export" code is mostly maintained in separate packages ... and that in mne-python there is just a "thin API wrapper"

see: https://github.com/mne-tools/mne-python/blob/614424718fa911cc0cfacc0df85652b3efaee358/mne/export/_brainvision.py#L1-L20

[cbrnr]

I see, but the "thin" wrapper code should also include these helper functions IMO. I don't see a reason why pybv is supposed to maintain it.

mne-tools/mne-python#12450

[hoechenberger]

Hello! I was brought here because I saw the PR/discussion over at the MNE repository

I have to say I find it a little strange that pybv is shipping some code that is exclusively being used by MNE-Python – not even pybv itself! – just so that MNE doesn't need to ship that code? It's literally a bunch of private functions that are not used to operate the public API. And looking at the respective unit tests – we're testing MNE functionality here, in pybv unit tests. It's just the weirdest thing … This all reeks of a massive anti-pattern to me. Or maybe I'm misunderstanding some bits, which is very well possible :D

Just my 2 ct.

[larsoner]

I think pybv should expose a _export_mne(raw) or similar. At the MNE end we shouldn't use anything other than this from pybv. And at the pybv end only public MNE functions and interfaces should be used. This is the most reasonable separation to me.

Ideally if there is a bug with export_bv then the fix lives exclusively in the pybv package. If it's not that way now, we should move more in that direction. This PR takes us in the other direction.

[cbrnr]

I'd argue that we should do the exact opposite. pybv is a tool that can be used by many packages, because it only depends on NumPy. MNE-specific functionality required to convert arguments should go into MNE.

[hoechenberger]

I'd argue that we should do the exact opposite. pybv is a tool that can be used by many packages, because it only depends on NumPy. MNE-specific functionality required to convert arguments should go into MNE.

I'd argue the same, yes

But I'm not voting for or against anything here, just wanted to share my thoughts :D

[larsoner]

Unfortunately we decided the opposite. We had to choose "who should maintain code to take an MNE raw and write a different format" and we decided the answer was "other modules, not MNE."

[larsoner]

Unfortunately for this PR I mean. I think it's the right choice for MNE 😄

[sappelhoff]

The current situation in pybv serves two purposes:

1. cater to the choice that was made for MNE-Python by providing export functionality
2. but by keeping this functionality private, we don't unnecessarily extend the scope of pybv for a potential end user

Re: "2." --> I just think we shouldn't have an export_mne (public!) function in pybv. It's completely sufficient to only have this (i.e., the public brainvision export) in MNE-Python ... hence keeping all of it private in pybv.

I have to say I find it a little strange that pybv is shipping some code that is exclusively being used by MNE-Python – not even pybv itself! – just so that MNE doesn't need to ship that code? It's literally a bunch of private functions that are not used to operate the public API. And looking at the respective unit tests – we're testing MNE functionality here, in pybv unit tests. It's just the weirdest thing … This all reeks of a massive anti-pattern to me.

I know what you mean @hoechenberger but it's really the result of points 1. and 2. in my post here (see above).

I think it's the right choice for MNE

I also think that the present choice of maintaining export code in third-party packages (like pybv or eeglabio) has its advantages. Mostly because there are so many data formats, it's impossible for MNE-Python core devs to be familiar with all of them, so it makes sense to leave that to people who know about it and care about it (e.g., pybv devs) ... and then as a boon for these people (and all end users), make it easy for everyone to use these functions from MNE-Python.

---

Having that said, ... it is a bit weird to have all this private code and MNE-Python testing in pybv. But I don't know a better solution that wouldn't disagree with the fundamental MNE-Python "export choice".

[cbrnr]

Having that said, ... it is a bit weird to have all this private code and MNE-Python testing in pybv. But I don't know a better solution that wouldn't disagree with the fundamental MNE-Python "export choice".

I find it not just a bit weird, but just plain silly. The fundamental export choice of MNE-Python still stands if it included helper functions that convert MNE objects to the format expected by the export packages. This is also what is currently being done for EEGLAB, EDF, and MFF.

Let's maybe continue the discussion at mne-tools/mne-python#12450 and see if we can get to an agreement.

[sappelhoff]

Let's maybe continue the discussion at mne-tools/mne-python#12450 and see if we can get to an agreement.

👍

[cbrnr]

Please do not merge until the next MNE release v1.7.0 (which I think should be around the corner). Or at least pybv should not create a new release until then.

[cbrnr]

MNE v1.7.0 is out, so this is ready for merge.

[sappelhoff]

Cool, one final check though: It seems to me that we lost the roundtrip tests, as they weren't added here: mne-tools/mne-python#12450

do you think they are unnecessary?

[cbrnr]

MNE already has a similar roundtrip test here: https://github.com/mne-tools/mne-python/blob/main/mne/export/tests/test_export.py#L56

Should we replace this with the test from pybv? I'm not sure how to best combine both tests so as to not lose anything important.

[sappelhoff]

After some inspection, I think there is not a lot that we'll be missing:

• some checks that warnings are being raised (for overwrite param, and for containing "vhdr" in fname)
• single vs double data
• temperature channel in Celsius

I am going to merge this, as I consider this fine. Feel free to add these features to the mne test (whoever cares about it) in the future :-)

Thanks @cbrnr