Include helper functions from pybv

[cbrnr]

I think it makes sense to maintain helper functions used to prepare input for pybv from Raw objects in MNE instead of pybv. See bids-standard/pybv#120.

BTW, the test does not compare annotations in the original and exported/imported file, probably because our BrainVision reader does not support channel-specific markers yet?

[cbrnr]

Qt acting up in some of the tests? I don't think these are related.

[sappelhoff]

I am fine either maintaining it here or in pybv.

[larsoner]

Hopefully #12452 takes care of the CI problems

[larsoner]

@cbrnr from bids-standard/pybv#120 (comment) you said

I don't see a reason why pybv is supposed to maintain it.

IIRC the idea is/was to keep as much export maintainance outside MNE-Python as possible. So the people most familiar with how to write BV files should maintain the BV stuff in pybv, those most familiar with EEGLAB in eeglabio, etc. At some point we decided this was the way to go after discussing the issue. Can you look back at the GitHub history and find the relevant discussions?

[cbrnr]

I cannot find a discussion, only you mentioning that this is how it was decided 😄:

#9427
#10678 (comment)

I'm fine with the idea to outsource as much export maintenance as possible, but I think it shouldn't be too much. In the case of pybv, I'd argue that functions that exclusively prepare MNE data to feed into pybv should really live in MNE, and not in a private module in pybv. Also, the maintainers are also contributors to MNE, so it's not going to be a huge problem I think 😉.

[larsoner]

I'm fine with the idea to outsource as much export maintenance as possible, but I think it shouldn't be too much. In the case of pybv, I'd argue that functions that exclusively prepare MNE data to feed into pybv should really live in MNE, and not in a private module in pybv.

Yeah unfortunately I think this goes against the current/existing plan, which I think can be articulated as roughly 1) keep stuff like this in external modules, but 2) modify MNE with new public methods if necessary information is not in public interfaces. I'd rather not change course -- I don't see a compelling reason to do so.

[cbrnr]

1. keep stuff like this in external modules

Are you saying that we delegate completely internal MNE stuff to external modules?

2. modify MNE with new public methods if necessary information is not in public interfaces

OK, so you are suggesting to make the function _export_mne_raw() public?

[larsoner]

I think all of this code belongs in pybv. The only thing we should do at the MNE end ideally is call pybv._export_mne_raw(raw). That being public or private at the pybv end doesn't matter to me, fine by me to have it be an internal API agreement that MNE will use that private function not meant for end users.

[cbrnr]

Eric, I don't really get it. It looks like you view pybv as some MNE helper package, which it clearly isn't, at least to me.

[larsoner]

To summarize how (I think at least, it was some years ago!) we decided to go this "punt to other packages as much as possible" idea, the main problem we're trying to solve I'd frame as: end users want to take mne.io.(Base)Raw instances write them in N different output formats. For years we only gave supported N=1, i.e., writing FIF. But people reasonably wanted to write to N>1 formats. You could imagine adopting one of two extremes at that point:

1. Say "we will not support writing anything other than FIF", and then pybv, eeglabio, etc. (or some other packages!) would have to do absolutely everything, with no interface, coordination, or anything provided by MNE-Python. (I guess this would be the extreme of the "helper package" idea?)
2. Say "we will provide first-party support for any output format (with sufficient demand) in MNE", and have all other-format-writing code live in MNE.

I agree something toward either of these extremes would be the cleanest option from a conceptual standpoint, and that there are lots of options you could go for in between these them in terms of who is responsible for what. So how did we end up where I think we did, which is much more toward the first option?

Practically, MNE-Python maintainers have finite, limited bandwidth and knowledge, so we have to make compromises. Adding exporting functionality makes our (already too big!) codebase bigger. The more stuff we can ask other modules and maintainers to do, the better from the perspective of the health of MNE-Python. Have a problem with eeglab export? Open an issue on eeglabio. BrainVision? Open an issue on pybv. And those issues can be solved independently of MNE-Python, arguably by the people best qualified to take care of them (the format experts). I think this is a pretty reasonable approach, and it will help with our package sustainability to try to keep going in this direction.

Where some conceptual impurity comes in is perhaps with us supporting a mne.io.export interface at all. However, the practical cost at our end is very low in providing a unified mne.export.* set of functions , and these have the benefit to end users, and also libraries that can help us support exporting.

[cbrnr]

I'm not arguing against our decision to outsource the actual exporting part to external packages. What I find strange is that only in the case of pybv, MNE is not converting MNE-specific objects to a format required for the export function. This is not the case for exporting to EEGLAB, EDF, and MFF. The first two formats also use external packages to do the heavy lifting (eeglabio and edfio), but MNE-Python includes some private functions that prepare the data to be compatible with the required arguments for these functions. I think EEGLAB export should do the same.

Your argument of finite dev bandwidth is equally valid if we move these two private functions into MNE. There are very few (if any?) MNE maintainers who are comfortable with all provided functionality, and this has never been a problem. Also, all pybv maintainers are MNE contributors anyway, and I'd rather work on MNE-specific issues directly in MNE.

And again, I am not disputing the idea of delegating the actual export to other packages. But I have a problem with including functions that are exclusively used by MNE-Python in pybv as a private module.

[sappelhoff]

Having slept over it and based on the discussion, I switch from my "either way is fine" position to being in favor of maintaining the code that converts from mne RAW to inputs as required by pybv within MNE-Python, as is already done for a number of other formats. This is what @cbrnr suggests in this PR, and I think the "maintenance burden" is very low -- especially when exclusively public (well documented) functions of pybv are being used.

As for testing, all io and sanity tests should happen in pybv, except for maybe one or two basic tests in mne-python, that make sure that the actual conversion looks fine.

so +1 for this PR, and deleting the corresponding private module in pybv.

[mscheltienne]

I'm in favor of Eric, IMO, if the MNE-code used by pybv is only used in pybv and not in other export-package, it should live in pybv. If however it is used in multiple export package, then it should live in mne (i.e. if it can be generalized).
In the end, I would prefer to move package-specific code from MNE to those package to simplify MNE's codebase (e.g. eeglabio, edfio).

I'm not very familiar with the export package and with who depends on who; but if the idea is that those packages are not only used by MNE-Python and thus should not depend on MNE, then I would turn mne into an optional dependency for those export package at their end.

[cbrnr]

@mscheltienne this is like asking pandas to include functions that convert mne.io.Raw in their package, so that MNE can simply call this function directly. Do you think this makes sense?

[mscheltienne]

No, but I would argue the larger package wins and we should try to simplify the codebase and maintenance of the larger package. In your example, pandas, in this PR, mne.

[hoechenberger]

Asking a non-MNE package to include MNE code to avoid shipping it with MNE itself has got to be the weirdest design decision we've made in a looong time 🥹🤦

Edit: No, actually the one where we recently re-implemented language support for keyword arguments ourselves is the uncontested winner. This one here is the runner-up though ☺️🥈

[larsoner]

It sounds like @mscheltienne and I are out outvoted on this one by @cbrnr @sappelhoff and @hoechenberger , so I can live with moving in the direction suggested by @cbrnr . We should wait for @drammock to chime in though (next week most likely) before proceeding

[drammock]

Setting aside for a moment the fact that many of the same individuals sit on both sides of this issue (looking at you @cbrnr 🙂), I think that as MNE devs we have no choice but to respect the decisions made by the maintainers of other packages in our ecosystem. When Sphinx devs make changes that break our docs build, we have basically 3 choices: (1) open an issue and/or PR to (persuade Sphinx devs to) change Sphinx; (2) change MNE to adapt to the change in Sphinx; or (3) abandon Sphinx and build our docs some other way. (not trying to pick on Sphinx --- just using it as an example that is unrelated to the particular changes discussed here).

There's a gray area between code that clearly belongs in MNE and code that clearly belongs in format-specific read/write/manipulate packages. If the pybv maintainers say "we want this package to only handle read/write/manipulate operations and not cross-package conversion of in-memory representations" then we can't force their hand. I don't agree that it's "silly" for a package like pybv to also include code that converts MNE's objects into their preferred internal representation --- after all, we do that for Pandas DataFrames! --- but it's a choice that pybv devs get to make.

That said, I don't agree with the proposed direction here (now comes the "persuasion" part). I can see @cbrnr's point that we do a fair amount of output prep in mne/export/_edf.py and mne/export/_eeglab.py that we're not doing for Brainvision export. In that sense we're not being consistent, and this PR improves consistency. But (reductio ad absurdum) it would be even more consistent if MNE absorbed even more of the functionality of pybv, eeglabio, etc: after all, we have a file writer for FIFF! why not maintain file writers for other formats? And we already handle Brainvision reading, why not writing? That way of thinking is obviously unsustainable and we have to draw lines somewhere.

If the desired end-state here is "cleaner separation of concerns" / "pybv only handles brainvision stuff, not MNE stuff" then I think there's a strong case that pybv should absorb MNE-Python's brainvision reading code (🥫🪱!). Whether pybv should also handle conversion of MNE objects into a pybv-writer-friendly format is, again, a matter of opinion and the pybv dev's choice --- but it's not clear to me that it's actually demonstrably easier to maintain if that code is in MNE versus in pybv. My opinion is that @mscheltienne's logic of "the smaller package/community should adapt to the bigger one" is, for better or worse, a good rule of thumb and applies here. If pybv devs choose to merge bids-standard/pybv#120 then we need to adapt somehow, and my reluctant vote would be to adopt that code into MNE.

However, note that moving the code from pybv to MNE also creates the risk that users will end up in a state where they can't write BV format at all, if they have an older MNE version and a newer pybv version. The only way I can think to prevent this would be if pybv added MNE as a required dependency and put a lower pin on its version --- but I'm sure that's a non-starter idea right? So that means pybv devs and MNE devs will need to do some extra documentation/support work around this transition --- and for what gain? Aesthetic satisfaction that pybv has an even smaller codebase than it already has, and that mne/export/_brainvision.py is approximately the same number of LOC as mne/export/_eeglab.py? Doesn't seem worth it to me.

[cbrnr]

I really understand your points, but in this case the better solution for me is still to include the preparation part within MNE. Here's a summary of my reasons:

1. pybv is a package for writing BrainVision data with minimal dependencies. It lives within the bids-standard organization, so it is not part of the MNE ecosystem.
2. MNE developers know best how to work with MNE objects, so anything related to wrangling MNE objects as a preparation step before exporting should be done within MNE.
3. Consistency with other export formats (EDF, EEGLAB).
4. The actual exporting functionality remains in third-party packages.
5. As @drammock mentioned, @sappelhoff and I are both developers of the pybv and mne packages. Instead of ignoring this fact, we should embrace it. We will address any issues with BrainVision export no matter where the data preparation step is located.

You will probably still disagree, but as a developer in both projects, I think this is the best solution for both packages. Therefore, I would like to go ahead with this change and hope that you can live with it.

[sappelhoff]

I think there are good arguments on both sides. For me as a pybv maintainer, these two points are very important:

1. Preferably no introduction of further dependencies in pybv --> pybv should remain "lightweight"
2. IF we supply and maintain mne functionality in pybv, it should/must be private and not for the end-user to see --> this is the current solution: All of that code is handled in a private module and not documented in user facing docs ... and this is important to me again to remain "lightweight" (at least from the perspective of the user)

I am personally fine with point 2. (and the reasons why it's beneficial for MNE-Python), but I understand Richard's and Clemens' points as well. If I were forced to vote, I'd reluctantly tend towards accepting the present PR.

Regarding this point:

However, note that moving the code from pybv to MNE also creates the risk that users will end up in a state where they can't write BV format at all, if they have an older MNE version and a newer pybv version. The only way I can think to prevent this would be if pybv added MNE as a required dependency and put a lower pin on its version

This risk will be minimal / non-existent due to the high overlap between mne an pybv devs and our awareness of this potential situation.

[drammock]

Well, the pybv devs have spoken. I won't stand in the way of this PR. I'm still worried about this point:

moving the code from pybv to MNE also creates the risk that users will end up in a state where they can't write BV format at all, if they have an older MNE version and a newer pybv version.

@sappelhoff is not worried because

This risk will be minimal / non-existent due to the high overlap between mne an pybv devs and our awareness of this potential situation.

... but after bids-standard/pybv#120 is merged and a new release of pybv is on PyPI, I don't see how "developer overlap between the projects" prevents users from doing something like conda install mne=1.6 pybv and ending up with an environment that can't write brainvision files --- MNE 1.6 won't have an upper pybv pin, and it won't have this PR's contents, so it will try to import pybv._export._export_mne_raw and it will fail. Please feel free to enlighten me as to why I'm wrong about this.

[cbrnr]

Then let's merge only this PR, and we wait until the next MNE release before merging the corresponding PR in pybv.

[drammock]

Then let's merge only this PR, and we wait until the next MNE release before merging the corresponding PR in pybv.

That doesn't completely eliminate the risk, but does reduce it to a level I'm more comfortable with. Good idea.

[larsoner]

Thanks @cbrnr !