detected breaks not only if diffs are positive, but use abs(diffs)

[behinger]

Fixes #137

I did not run tests locally because I dont have the tooling installed

[cbrnr]

@jamieforth can you take a look? @behinger if you fix the style errors we'll see if the tests succeed (or if they need to be adapted).

[behinger]

that was surprisingly easy to do

[jamieforth]

I don't think I can push to this branch but there's updated tests here: behinger#1. All tests pass except for one: test_jitter_removal_glitch involving the calculation of effective sample-rate.

I still tend to think this is more of a hack for a broken LSL device than a bug in pyxdf, but abs(diff) is a nice practical solution (it was also my first thought too #127). However, it can cause subtle problems with parts of the code (or subsequent resampling, analysis etc.) that assume monotonicity. It's probably possible to adapt other code to not assume time-stamps are always strictly ordered, but it's a very convenient assumption to be able to rely upon.

The failing test is one example: it fails because the following timestamps are now segmented into three segments.

[1, 2, 3, 4] + [6, 5] + [7, 8, 9, 10]

The duration of the middle segment is -1, which leads to the incorrect calculation of effective sample rate.

We could change the test if we deem this to be the correct behaviour?

[cbrnr]

I agree with @jamieforth that this change should probably not replace the current behavior if the issue is the device. I didn't follow the entire discussion, but would it not be better to write a small tool to fix the broken timestamps in the first place?

[behinger]

No, It's not the device. This error can happen everywhere in case jitter is larger than 1/fs

I explained it here: #137 (comment)

I think one misunderstanding is that this is depending on ANT, but I use timestamps from lsl/laptop not the device itself when sending the chunks.

[jamieforth]

No, It's not the device.

By device I mean hardware plus LSL wrapper software. The interaction between these two parts of the "LSL device" is where I think this problem arises.

I use timestamps from lsl/laptop not the device itself when sending the chunks.

This is how LSL is designed to work in the most straightforward case - synchronisation is only possible with time-stamps relative to the LSL clock (from your laptop).

This error can happen everywhere in case jitter is larger than 1/fs

I agree, but I'd also take jitter of that order as evidence of a problem at the hardware ↔ LSL wrapper level. I haven't seen jitter anywhere near 1/fs for (the albeit small number of) other devices I've used.

Being pragmatic, abs(diff) is a simple way of handling this (to some extent, i.e. notwithstanding the caveat above that it can lead to other problems down the line if you're doing any kind of analysis that assumes timestamp intervals are always positive).

It's also what xdf-matlab decided to do, so perhaps that's a good enough reason to adopt it too?

If so, what should we do with the test_jitter_removal_glitch test? I think the options are:

1. we say segments with negative duration are correct behaviour and change the test to pass given the new behaviour;
2. or, we mark it as expected to fail and just consider it an obscure edge case.

[cbrnr]

I'm OK with doing what the MATLAB lib does. It still seems like it's probably an upstream issue, but let's just be consistent with MATLAB here.

Regarding the test, I'll leave it up to you, though I'd slightly prefer option 2. But it's your call!

[behinger]

I have seen jitter > 1/fs on other computers with pylsl - maybe a function of buffersize? But in any case, keeping same as matlab is good

[jamieforth]

Cool, xfail for the test was my instinct too.

@behinger I think you'll need to merge my PR in your fork to get the updated tests here, unless @cbrnr has a smarter suggestion.

[behinger]

argh super sorry - I missed this somehow. Merged now!

[cbrnr]

Thanks @behinger and @jamieforth!