Skip to content

Conversation

@drammock
Copy link
Member

PR Description

EDF/BDF formats restrict the startdate (equivalent of meas_date) to be 1985-01-01 or later, which conflicts with the BIDS standard for anonymization (which requires anonymized dates to be before 1920-01-01). In #669 a decision was made to write EDF(+) files that violate the BIDS standard but honor the EDF standard (making them viewable in EDFBrowser), while writing the BIDS-compliant anonymized date in scans.tsv. However, it was only implemented in cases where the anonymized data started out in EDF format (i.e., only mne_bids/copyfiles.py was changed). In cases where the data start in another format, are anonymized, and then written to EDF(+), the result will be an invalid EDF file. This PR fixes that. As with #669, the BIDS-compliant anonymized date will still be written to scans.tsv; only the EDF file itself will be modified.

The PR also does a small internal refactor to allow both EDF and BDF writing; this is needed for #1129, because EMG modality will have BDF(+) as a RECOMMENDED format.

Merge checklist

Maintainer, please confirm the following before merging.
If applicable:

  • All comments are resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • New contributors have been added to CITATION.cff
  • PR description includes phrase "closes <#issue-number>"

@codecov
Copy link

codecov bot commented Nov 13, 2025

Codecov Report

❌ Patch coverage is 92.30769% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.96%. Comparing base (dd3c984) to head (d9018a2).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
mne_bids/tests/test_write.py 90.90% 2 Missing ⚠️
mne_bids/write.py 83.33% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1479   +/-   ##
=======================================
  Coverage   96.96%   96.96%           
=======================================
  Files          43       43           
  Lines       10100    10119   +19     
=======================================
+ Hits         9793     9812   +19     
  Misses        307      307           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@drammock
Copy link
Member Author

drammock commented Nov 13, 2025

arg, still getting mne_bids/tests/test_fileio.py::test_lock_refcount_multiprocess - AssertionError

https://github.com/mne-tools/mne-bids/actions/runs/19348373149/job/55354332061?pr=1479#step:18:541

@bruAristimunha

@drammock
Copy link
Member Author

@sappelhoff there's a stdlib feature of datetime module only available on python 3.11+, making the 3.10 CIs fail

https://github.com/mne-tools/mne-bids/actions/runs/19348373149/job/55354332026?pr=1479#step:22:376

The string is valid ISO format, this was a bug-ish in python prior to 3.11. Is this justification enough to:

  1. bump the min CIs to use 3.11
  2. bump the package min python version to 3.11
  3. neither (1) or (2), just put a guard on that particular test

?

@sappelhoff
Copy link
Member

I would be happy to have the min python pushed to 3.11.

It's still 11 months to go until 3.10's EOL, but we can also make our lives a tiny bit easier.

@drammock
Copy link
Member Author

I would be happy to have the min python pushed to 3.11.

will do

It's still 11 months to go until 3.10's EOL [...]

FWIW SPEC-0 would recommend already setting 3.12 as the minimum. So switching from 3.10 to 3.11 is already a bit conservative from the perspective of our ecosystem peers.

@drammock
Copy link
Member Author

drammock commented Nov 14, 2025

OK CIs are happy except for one more case of
mne_bids/tests/test_fileio.py:545: in test_lock_refcount_multiprocess (which we're handling separately in #1480)
. The 6 "pending" checks are cruft due to the CI job names changing when I changed the Python version, and the codecov/patch result is spurious (no new missed lines).

Ready for review/merge

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.

Thanks a lot @drammock

@sappelhoff sappelhoff enabled auto-merge (squash) November 14, 2025 17:49
@sappelhoff
Copy link
Member

Ah
image

this actually needs some changes to our required checks: https://github.com/mne-tools/mne-bids/settings/branch_protection_rules/2696889

image

I can have a look at this soon

@sappelhoff sappelhoff merged commit 8e4e13d into mne-tools:main Nov 14, 2025
23 of 24 checks passed
@drammock
Copy link
Member Author

@sappelhoff I updated the branch protection rules to replace the 3.10 jobs with the corresponding 3.11 jobs.

@drammock drammock deleted the edfanon branch November 14, 2025 18:05
@sappelhoff
Copy link
Member

Thank you, Dan.

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.

2 participants