Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible bug with _clock_sync and clock reset detection #93

Open
JakubOrlinski97 opened this issue Feb 15, 2023 · 5 comments
Open

Possible bug with _clock_sync and clock reset detection #93

JakubOrlinski97 opened this issue Feb 15, 2023 · 5 comments

Comments

@JakubOrlinski97
Copy link

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

@cbrnr
Copy link
Contributor

cbrnr commented Feb 16, 2023

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

@cbrnr
Copy link
Contributor

cbrnr commented Jul 18, 2024

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 cbrnr closed this as completed Jul 18, 2024
@andbiz
Copy link

andbiz commented Dec 6, 2024

Hi, sorry to re-open this but I think @JakubOrlinski97 was correct in reporting the bug in this (

r = slice(range_i[0], range_i[1])
) 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.

@cbrnr cbrnr reopened this Dec 6, 2024
@andbiz
Copy link

andbiz commented Dec 6, 2024

pyxdf.txt

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

@chkothe
Copy link
Contributor

chkothe commented Dec 6, 2024

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants