Skip to content
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

Remove MNE export #120

Merged
merged 3 commits into from
Apr 21, 2024
Merged

Remove MNE export #120

merged 3 commits into from
Apr 21, 2024

Conversation

cbrnr
Copy link
Collaborator

@cbrnr cbrnr commented Feb 19, 2024

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.

Copy link

codecov bot commented Feb 19, 2024

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.

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

cbrnr commented Feb 19, 2024

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
Copy link
Collaborator

hoechenberger commented Feb 19, 2024

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
Copy link

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
Copy link
Collaborator Author

cbrnr commented Feb 19, 2024

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
Copy link
Collaborator

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
Copy link

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
Copy link

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

@sappelhoff
Copy link
Member

sappelhoff commented Feb 19, 2024

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
Copy link
Collaborator Author

cbrnr commented Feb 20, 2024

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
Copy link
Member

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

👍

@sappelhoff sappelhoff marked this pull request as draft February 27, 2024 17:37
@cbrnr
Copy link
Collaborator Author

cbrnr commented Mar 1, 2024

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 cbrnr marked this pull request as ready for review April 20, 2024 07:08
@cbrnr
Copy link
Collaborator Author

cbrnr commented Apr 20, 2024

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

@sappelhoff
Copy link
Member

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
Copy link
Collaborator Author

cbrnr commented Apr 21, 2024

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.

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sappelhoff sappelhoff merged commit 5f5732a into bids-standard:main Apr 21, 2024
16 checks passed
@cbrnr cbrnr deleted the remove-mne-export branch April 22, 2024 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why is _export_mne_raw() private?
4 participants