pyxdf icon indicating copy to clipboard operation
pyxdf copied to clipboard

Possible bug with _clock_sync and clock reset detection

Open JakubOrlinski97 opened this issue 2 years ago • 1 comments

When looking through the code of _clock_sync (https://github.com/xdf-modules/pyxdf/blob/main/pyxdf/pyxdf.py#L538) which is used at import time to synchronise clocks between streams I think I found a bug.

In the synchronisation method there is a part that detects clock resets, and then processes each chunk of data separately if there is significant differences between the clock offsets.

The problem is, when the coefficients for the linear function are taken and applied to the timestamps, the code uses indexes from the range determined from the clock_values. These are not the same indexes as those that should be used to address the timestamps of the recording. This means only a tiny subset of the recording gets synchronised as the clock_values get saved only every 5 seconds or so.

The relevant code is in this for loop, specifically where stream.time_stamps is addressed with the slice: https://github.com/xdf-modules/pyxdf/blob/main/pyxdf/pyxdf.py#L637

JakubOrlinski97 avatar Feb 15 '23 13:02 JakubOrlinski97

Can you provide a short example which shows the problem (ideally with a suitable XDF file)? What would be your proposed fix?

cbrnr avatar Feb 16 '23 11:02 cbrnr

I am closing this issue due to the original poster's lack of response to the follow-up question. @chkothe do you have any opinion on whether the current clock sync algorithm might be buggy?

cbrnr avatar Jul 18 '24 09:07 cbrnr

Hi, sorry to re-open this but I think @JakubOrlinski97 was correct in reporting the bug in this (https://github.com/xdf-modules/pyxdf/blob/0df1599904cd6a28918705ca2a377afeb228eb46/pyxdf/pyxdf.py#L621) loop.

As far as I understand, range_i[0], range_i[1] are the indexes in clock_times used to segment portions of clock_times between clock resets. Those indexes are not appropriate to segment stream.time_stamps.

I assume we need an additional step:

  • given the time interval defined by clock_times[range_i[0]] and clock_times[range_i[1]]
  • identify the portion of stream.time_stamps inside that interval and obtain the respective start and stop indexes (e.g. idx_start, idx_end
  • create the slice based on idx_start and idx_end: r = slice(idx_start, idx_end).

Does it make sense to you? I can privately provide an xdf file for testing.

andbiz avatar Dec 06 '24 12:12 andbiz

pyxdf.txt

See if it helps. It worked well on my data.

andbiz avatar Dec 06 '24 13:12 andbiz

Yes, this looks like a bug in the case where a PC clock resets or jumps during a recording. I think @andbiz and @JakubOrlinski97 share the price for the first users who actually encountered this. And yes the proposed fix sounds reasonable, but we'll want to make sure we don't have an off-by-one error at one of the segment edges, so having that test file would be useful. Is that something you got in an actual recording? (would be curious what that looks like in the data and how large the jump was)

chkothe avatar Dec 06 '24 16:12 chkothe