search

holgern / pyedflib

pyedflib is a python library to read/write EDF+/BDF+ files based on EDFlib.
http://pyedflib.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
209 stars 121 forks source link

WARNING: Sample frequencies may be incorrect!!! #197

Closed NarayanSchuetz closed 1 year ago

NarayanSchuetz commented 1 year ago

Almost lost my sanity finding this. So it turns out that newer versions of this library have a different way to calculate the sampling frequency, which may result in wrong sampling frequencies. Likely a result of commit 58f3e3589c7b04dac7726bb78a8f687c63f94d1e.

Now the EDF files we were using are generated by a proprietary Application, which we have no control over. The manufacturer may have messed up something that went unnoticed because they tested against the old library or maybe there are deeper issues with the current implementation.

As I'm not familiar with the reasoning behind the changes, I cannot say with certainty, but to me, this looks like a critical breaking change that should be reverted if anyhow possible as I suspect this problem may not be limited to just the Software we are using (given EDF is used as an export format with many older medical devices). At the very least users should be notified by a warning, because this is very difficult to debug.

If it is wanted, I'm happy to provide a pull request with a variant where both the old and new variants to calculate the sampling frequencies are used and a warning is raised if they do not match.

Version 0.1.19:

Pasted Graphic

Version 0.1.30

Pasted Graphic 1
NarayanSchuetz commented 1 year ago

Just saw this has already been reported, please at least include a warning, this is a critical bug and people should at the very least be aware!

skjerns commented 1 year ago

I think this is fixed in the Master version, could you check?

pip install git+https://github.com/holgern/pyedflib

We are currently waiting for the owner of the repository to draft a new release @holgern , but I cannot reach him :-/

unfortunately without a release, we also cannot include any warning

holgern commented 1 year ago

Sorry, I will now work on automatic pypi upload using github actions.

NarayanSchuetz commented 1 year ago

@skjerns

Yes, the Master version seems to work as expected, at least for me! Thanks for taking care of the issue.

skjerns commented 1 year ago

Perfekt, glad to hear! (and kind of reliefed that there is no major overseen bug in the current version)

big thanks also to @holgern for making releases more easy :)