Skip to content

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Oct 7, 2025

Now that edfio 0.4.10 added support for BDF, we should support that in MNE as well. The current implementation duplicates a lot of EDF code, which I think is OK for now (I'd prefer to refactor in a follow-up PR).

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 7, 2025

Any ideas why the ultraslow_pg job is failing?

The failing Windows pip pre job is due to pyvistaqt DeprecationWarnings, so this is unrelated I guess. The Ubuntu pip pre job segfaults, also due to some VTK issue.

@cbrnr cbrnr added the backport-candidate on-merge: backport to maint/1.10 label Oct 7, 2025
@larsoner
Copy link
Member

larsoner commented Oct 7, 2025

The current implementation duplicates a lot of EDF code, which I think is OK for now (I'd prefer to refactor in a follow-up PR).

It looks like _bdf.py and _edf.py only differ by < 10 lines. Can I push a commit just to take care of this now? Seems simpler and cleaner than adding a bunch of lines just to almost immediately remove it

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 7, 2025

Sure, but if you want I can also refactor it.

@larsoner
Copy link
Member

larsoner commented Oct 7, 2025

Sure feel free! On Azure this failure does look related:

_____________________ ERROR collecting mne/export/_bdf.py ______________________
mne/export/_bdf.py:13: in <module>
    _check_edfio_installed()
mne/utils/check.py:451: in _check_edfio_installed
    return _soft_import("edfio", "exporting to EDF", strict=strict)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
mne/utils/check.py:429: in _soft_import
    raise RuntimeError(
E   RuntimeError: For exporting to EDF to work, the module edfio is needed, but it could not be imported. Use the following installation method appropriate for your environment:
E   
E       pip install edfio
E       conda install -c conda-forge edfio
---------- generated xml file: /home/vsts/work/1/s/junit-results.xml -----------
=========================== short test summary info ============================
ERROR mne/export/_bdf.py - RuntimeError: For exporting to EDF to work, the module edfio is needed, but it could not be imported. Use the following installation method appropriate for your environment:

    pip install edfio
    conda install -c conda-forge edfio

but maybe the refactoring already took care of it. I'll tackle the unrelated errors in #13434

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 7, 2025

I don't understand this error. The new test_export_raw_bdf function has the exact same markers as test_export_raw_edf (i.e., @edfio_mark() and pytest.mark.slowtest in particular). So why is this test collected in the first place?

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 7, 2025

Found it and hopefully fixed it.

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 7, 2025

Looks good, feel free to merge @larsoner!

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple minor things

@drammock drammock merged commit 61bc8b8 into mne-tools:main Oct 8, 2025
32 checks passed
Copy link

lumberbot-app bot commented Oct 8, 2025

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout maint/1.10
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 61bc8b8755da982491d4cebf940bbde160b98e1a
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #13435: Add BDF export'
  1. Push to a named branch:
git push YOURFORK maint/1.10:auto-backport-of-pr-13435-on-maint/1.10
  1. Create a PR against branch maint/1.10, I would have named this PR:

"Backport PR #13435 on branch maint/1.10 (Add BDF export)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 8, 2025

I'm going to give the backporting a try. Is there any chance we could release v1.10.2 anytime soon?

@cbrnr cbrnr deleted the bdf branch October 8, 2025 06:31
cbrnr added a commit to cbrnr/mne-python that referenced this pull request Oct 8, 2025
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Eric Larson <[email protected]>
(cherry picked from commit 61bc8b8)
@cbrnr cbrnr removed backport-candidate on-merge: backport to maint/1.10 Still Needs Manual Backport labels Oct 8, 2025
@cbrnr
Copy link
Contributor Author

cbrnr commented Oct 8, 2025

OK, I hope that worked! It would be great if someone could double-check though, as this was my first backport.

@drammock
Copy link
Member

drammock commented Oct 8, 2025

OK, I hope that worked! It would be great if someone could double-check though, as this was my first backport.

looks clean to me. Thanks for backporting. I think ideally you would have added a new section to https://github.com/mne-tools/mne-python/blob/main/doc/changes/v1.10.rst similar to what is done e.g. here:

.. _changes_1_7_1:
Version 1.7.1 (2024-06-14)
==========================
Bugfixes
--------
- Fix bug where :func:`mne.time_frequency.csd_multitaper`, :func:`mne.time_frequency.csd_fourier`, :func:`mne.time_frequency.csd_array_multitaper`, and :func:`mne.time_frequency.csd_array_fourier` would return cross-spectral densities with the ``fmin`` and ``fmax`` frequencies missing, by `Thomas Binns`_ (`#12633 <https://github.com/mne-tools/mne-python/pull/12633>`__)
- Fix incorrect RuntimeWarning (different channel filter settings) in EDF/BDF import, by `Clemens Brunner`_. (`#12661 <https://github.com/mne-tools/mne-python/pull/12661>`__)
Authors
-------
* Clemens Brunner
* Thomas Binns

(renders as https://mne.tools/dev/changes/v1.7.html)

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.

4 participants