Bug in start-time convertion

[burnpanck]

While trying to understand timezone information, I stumbled upon the following subtle bug: https://github.com/holgern/pyedflib/blob/1af9ced84921b304fecdc994e7e8b9a4c1674245/pyedflib/edfreader.py#L371-L372

Obviously, the conversion factor between nanoseconds and microseconds is 1000, not 100.

[skjerns]

Did you confirm this actually leads to a bug?

I remember something that edflib stores the time in units of 10ns, so the conversion is x100. I think this should be somewhere noted in the source code. Sorry I'm on mobile until next week, can't check it well

[skjerns]

See here

https://github.com/holgern/pyedflib/blob/1af9ced84921b304fecdc994e7e8b9a4c1674245/pyedflib/_extensions/_pyedflib.pyx#L634-L650

[burnpanck]

Did you confirm this actually leads to a bug?

No, I didn't. I kind-of did an "in-promptu code-review" here 😃. From a code-review perspective, those two lines are a clear issue in any case: either the code is wrong, or the comment is wrong.

The code that you have pointed out is not directly related to the start timestamp, but it does hint at a third unit which is neither nanoseconds nor microseconds. If self.starttime_subsecond were in units of 100 ns though as hinted at in the comment you linked, then the correct division factor would in fact be /10 and not /100 to get to microseconds, so the code would again be wrong.

In general, my recommendation would be to try to avoid spreading magic numbers like those 100 around in the code, and instead:

• try to minimise the number of different units used in the code
• wherever it is not super-obvious what unit could be meant, give the unit a name and include that unit name in the variable name (e.g. starttime_subsecond_ns or starttime_subsecond_edfticks)
• hide the magic numbers in constants that clearly state which units they convert (e.g. EDFLIB_NUMBER_OF_EDFTICKS_PER_NS)

If that is not feasible, then every magic number should be loudly and clearly documented (and the comment updated whenever the code is modified!). If there is a mismatch, it should be considered a bug.

That said, the whole EDF format family is too limited in it's capabilities for my taste: e.g. it seems to lack time-zone information. The actual EDF spec is very clear that starttime should be represented in the patient's local time, unfortunately, our design rules disallow naive timestamps entirely. We do need to ingest EDF files from the periphery sometimes, but we're trying to avoid it as much as we can.

Thus, feel free to proceed with this issue report as you see fit. The mismatch between the comment and the code caught my eyes, and I though you might want to know. If you don't, feel free to close.

[skjerns]

thanks! I fully agree with you :) I'll try to improve it in another cycle