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

API: Remove raw.impedances #12864

Open
larsoner opened this issue Sep 19, 2024 · 8 comments
Open

API: Remove raw.impedances #12864

larsoner opened this issue Sep 19, 2024 · 8 comments
Milestone

Comments

@larsoner
Copy link
Member

See discussion in #12861 (comment)

@larsoner
Copy link
Member Author

larsoner commented Sep 26, 2024

Argh @sappelhoff I went to look at pybv and the code there seems to have to do almost exclusively with writing, not reading BV data.

@sappelhoff WDYT about just keeping the code in MNE-Python with a mne.io.brainvision.read_impedances(...)?

@drammock
Copy link
Member

obligatory cross-ref to #12450 (comment), in particular

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

@larsoner
Copy link
Member Author

Oh okay with me then!

@drammock
Copy link
Member

Oh okay with me then!

unfortunately my suggestion in #12450 (comment) was not met with enthusiasm. But maybe now the case is stronger / more pursuasive? WDYT @cbrnr @sappelhoff?

@cbrnr
Copy link
Contributor

cbrnr commented Sep 27, 2024

I'd be OK with adding reader functionality to pybv, it does make a lot of sense. However, this would be the first format (I think) where we choose to do so, so why is this a problem suddenly? If the current reader function does read impedances (if present), and the only reason we're now removing it is because it doesn't survive a round-trip to FIFF, then IMO the problem is that we're still relying on FIFF. I'm pretty sure we've had this discussion before, but no one outside the small MEG community has ever used FIFF. It's a very niche file format that doesn't even have many readers outside of MNE. So why don't we think about moving to a new internal format that is independent of the FIFF idiosyncrasies, such as HDF? I think it could be as easy as dumping our current Raw into HDF, and then we'd be free to extend attributes even if FIFF does not support it.

Edit 1: It's not the first format, since we're using antio for both reading and writing.
Edit 2: Not true, even antio is using a reader from MNE.
Edit 3: OK, it's complicated 😄. The MNE reader needs antio, and antio has a reader that needs MNE... 🤔

@cbrnr
Copy link
Contributor

cbrnr commented Sep 27, 2024

PS: We're rolling our own EDF reader, even though edfio includes a reader and a writer. I'm absolutely not saying it is a bad thing that we implement our own readers (after all, we get to decide how strict they follow a given standard, which is not always desirable, etc.), so for me readers are not really up for discussion. It's the FIFF format that should be swapped with something better.

@mscheltienne
Copy link
Member

IMO the problem is that we're still relying on FIFF. I'm pretty sure we've had this discussion before, but no one outside the small MEG community has ever used FIFF. It's a very niche file format that doesn't even have many readers outside of MNE. So why don't we think about moving to a new internal format that is independent of the FIFF idiosyncrasies, such as HDF?

Agree, but that would be a major and complicated refactor to do.

@sappelhoff
Copy link
Member

Argh @sappelhoff I went to look at pybv and the code there seems to have to do almost exclusively with writing, not reading BV data.

Yes, that is true. I thought we'd want to very selectively outsource impedance reading to pybv, which would not be so hard to do. But it is true that is would be a bit odd that no reading is supported EXCEPT for impedances

@sappelhoff WDYT about just keeping the code in MNE-Python with a mne.io.brainvision.read_impedances(...)?

would be fine with me, too -- as long as we retain SOME way to get the impedances.

unfortunately my suggestion in #12450 (comment) was not met with enthusiasm. But maybe now the case is stronger / more pursuasive? WDYT

I would be fine with having pybv take over READING, as long as it supplies the read data in a "non-MNE" format (basic Python and numpy data structures), to stay "lightweight/minimal", and then have code in mne that turns these basic data structures into mne-ready structures.

However, it would require some work and effort that I cannot commit to at this point.

IMO the problem is that we're still relying on FIFF. I'm pretty sure we've had this discussion before, but no one outside the small MEG community has ever used FIFF. It's a very niche file format that doesn't even have many readers outside of MNE. So why don't we think about moving to a new internal format that is independent of the FIFF idiosyncrasies, such as HDF?

I agree with Clemens and Mathieu, the FIFF format often places an unnecessary limit on our plans of supporting more (or more detailed) (meta-) data. I am also aware that a potential change of the "saving backend" of mne would probably be the biggest change (labor intensive, error prone, ... but also valuable, and future-oriented) MNE-Python has seen in its current lifetime.

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

No branches or pull requests

5 participants