the-siesta-group / edfio

Read and write EDF/EDF+ files.
Apache License 2.0
27 stars 4 forks source link

When writing EDF, clip the signals to their actual digital max/min #1

Closed Teuniz closed 10 months ago

Teuniz commented 10 months ago

https://github.com/the-siesta-group/edfio/blob/2b2ca6734c83f862b96d633f70eee61712c68bf0/edfio/edf.py#L319

can potentially create invalid EDF files in case the default values for digital max/min aren't used.

It should be:

self._digital = np.minimum(np.maximum(np.round(data / gain - offset), self.digital_min), self.digital_max).astype(np.int16)

Remember, the digital max/min is the maximum/minimum value that can occur in a recording.

hofaflo commented 10 months ago

Hi, thanks for raising this issue! :)

Trying to write a testcase for it led to the realization that physical min and max are rounded in a way that can produce an overflow in the digital values. The fix proposed in #2 should avoid this and make clipping the values here redundant, in case I did not misunderstand the underlying issue?

Teuniz commented 10 months ago

Hi, thank you for your swift response.

My system's python version is too old for your library and I'm a bit busy at the moment but soon I hope to do some tests with this library myself. In the mean time, I would like to ask you if you have tested the following situation:

Using your library, create an EDF file with the following parameters: Digital maximum: 100 Digital minimum: -100 Physical maximum: 100 Physical minimum: -100 Write an array of floats containing all values of 300.0 and see what happens. It the samples in the file are clipped at 100, everything should be fine. If, however, the samples in the file have a value of 300 (or anything higher than 100), there's a problem.

Thanks for investigating!

hofaflo commented 10 months ago

Currently this raises an error saying that the signal range is out of the physical range (before anything is written). Users would have to manually clip the values before instantiating an EdfSignal. If automatic signal clipping would be helpful, we could e.g. add a parameter clip_data: bool = False to EdfSignal.__init__?

Teuniz commented 10 months ago

That's fine to me. From reading the code I wasn't sure if this is being checked (I'm not skilled in python). Thank you for (double)checking and confirming this :+1: Compliments for this library!