Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

Conda+mamba checks fail since edfio is not on conda-forge yet, I'll work on that.

edfio is not on conda-forge yet, I'll work on that.

Let me know if you need help, I did this for pysnirf2 just yesterday so it's fresh in my mind. But the short version is something like:

1. clone staged-recipes
2. pip install grayskull (or conda install it)
3. cd to staged-recipes/recipes
4. grayskull pypi edfio
5. Open PR to staged-recipes

Thank you for the simplified instructions!
Review is pending at conda-forge/staged-recipes#24511

* Two sections of `test_rawarray_edf` are removed, because
  
  * the patient birth date is formatted as "dd-MMM-yyyy" in EDF+, so a date like 20-JAN-1700 is actually valid
  * the export always uses EDF+C, which allows to store dates outside the range valid in "original" Edf (1985-01-01 to 2084-12-31) formatted as "dd-MMM-yyyy" in the "local recording identification" field

Pay attention. Allowing a measurement date outside the range 1985-01-01 to 2084-12-31 breaks backwards compatibility with EDF!
In order to protect this backwards compatibility, EDFbrowser will refuse to open an EDF+ file that has a startdate like, for example, 1700-01-01 or 2090-01-01.
(Ofcourse, this limitation in EDFbrowser will be removed before the year 2085 arrives.)

Thank you for the heads up @Teuniz! edfio currently stores non-EDF-compatible startdates according to EDF+ specs in the "local recording identification" field and sets the EDF startdate field to "01.01.85". On read, it prefers the EDF+ startdate to the EDF startdate, but of course that only makes sense if no other software is used. Since EDFbrowser does not open files with such a mismatch, we will update edfio to refuse writing them :)

Since EDFbrowser does not open files with such a mismatch, we will update edfio to refuse writing them :)

Thanks! In an "ideal" world where all EDF software has been updated to EDF+, this shouldn't be necessary,
but ofcourse this is not the case.
The goal is to avoid confusion when two different viewers (one EDF, the other EDF+) show different startdates.
Thanks again for your consideration. It's much appreciated.

Conda installs should work now, but I'll mark this as draft until the-siesta-group/edfio#5 is merged and released.

note: once this is merged, we'll have to make adjustments in mne-bids:

https://github.com/sappelhoff/mne-bids/blob/781fc577dc0da0419f7eb5c40d537b51834cb802/doc/install.rst?plain=1#L27

Just to be clear, even EDF+ mandates that the date range is between 1985–2084 (https://www.edfplus.info/specs/edfplus.html#additionalspecs). So regardless of EDFbrowser not opening it, this is non-standard and should therefore not be supported.

(Ofcourse, this limitation in EDFbrowser will be removed before the year 2085 arrives.)

The spec also says: "After 2084, yy must be 'yy' and only item 4 of this paragraph defines the date."

And this actually made my day 😄!

Another comment: we don't actually require all dependencies to be on conda-forge, so installing via pip would have also worked. But I guess it doesn't hurt to have it available over there as well.

There's a problem: edfio requires Python >= 3.9, but MNE currently still supports Python 3.8.

This should be relatively easy to fix though by dropping support for Python 3.8. Actually, since we're already adopting SPECS, SPEC 0 says we should drop 3.8 and 3.9 (and support only 3.10, 3.11, and 3.12).

Mind you our installers are often lagging behind by 1 Python release or so, because of the complicated mix of dependencies. Otherwise I'm all for cutting off things that are holding us back.

Is there already a timeline for dropping Python 3.8 support? If not – or if the plan is not to do that anytime soon – it should be relatively simple to support 3.8 in edfio. Let me know if that would be your preferred solution!

A few more things are fixed now:

• The dictionary mapping integers indicating sex in mne to letters used in EDF uses the mapping given here
• Empty subfields in patient/recording identification are substituted with "X" to ensure compatibility with EDF+
• Tolerances for comparing signal values and sample times to their values before exporting have been decreased to make passing the respective assertions non-trivial.
• Slicing signals to their original length has been removed where no padding occurs in the first place.

I'd prefer that we drop support for Python 3.8. This should be no problem, and we don't have to go full in and also drop 3.9. @drammock @larsoner OK?

We try to follow the scientific Python community standards for this and I think that means it's okay to drop 3.8 according to a quick look at https://numpy.org/neps/nep-0029-deprecation_policy.html#support-table

Let's push 1.6 out the door Monday or Tuesday (it's overdue) then drop 3.8 it while bumping dependency versions to 2-years-old. In the meantime you can ignore the "old" 3.8 CI

There's a problem: edfio requires Python >= 3.9, but MNE currently still supports Python 3.8.

This should be relatively easy to fix though by dropping support for Python 3.8. Actually, since we're already adopting SPECS, SPEC 0 says we should drop 3.8 and 3.9 (and support only 3.10, 3.11, and 3.12).

SPEC0 is even more aggressive, but I'm fine with dropping only 3.8 after the 1.6 release.

One more thing before I'm happy: exporting stimulus channels are not supported, right? For example, this test involves a Raw object with a stim channel, but it doesn't get exported. If this is intentional (I guess it is, but maybe we can take another look in another PR), I think we should at least emit a warning.

Good idea! Added a warning and removed the stim channel from all but one test to avoid checking for that warning in unrelated tests.

Great! Can you maybe remove this comment (# check channel types except for 'bio', which loses its type), which is incorrect (bio stays bio)?

This test is a bit convoluted, feel free to clean up a bit if it makes sense. For example, there's actually a third export (the very last line), which I'm not sure what it actually tests (just a smoke test, but for what). And the test is called test_double_export_edf(), which is not true because of the third export...

One more thing before I'm happy: exporting stimulus channels are not supported, right? For example, this test involves a Raw object with a stim channel, but it doesn't get exported. If this is intentional (I guess it is, but maybe we can take another look in another PR), I think we should at least emit a warning.

Because I'm curious, what's different about a stim channel that it cannot be saved in EDF+?

Because I'm curious, what's different about a stim channel that it cannot be saved in EDF+?

Good question, there's no reason why this cannot be done. In fact, this would be a perfectly valid and useful thing to do, but for some reason MNE-Python has excluded stim channels. We did not want to mess with that in this PR, but we'll tackle it in another PR. I guess the main difference between "normal" (physiological) and stim channels is that the latter contain exact (integer) values, but this can be dealt with by setting the physical and digital minima/maxima appropriately.

Done!

Perfect, thanks!

I agree with your comments, so let's wait until they are addressed.

BTW, will this land in 1.6.1 (backported), or do we wait until 1.7? I'd prefer backporting.

Done, thanks for the suggestions @larsoner!

Backporting to 1.6.1 would require support for Python3.8 in edfio, right?

Backporting to 1.6.1 would require support for Python3.8 in edfio, right?

True, I forgot. So I guess this will have to wait for v1.7 then (about 3 months).

We should have na "Dependencies" section in the changelog to mention changes of dependencies. @larsoner WDYT?

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

Thanks @hofaflo!