search

sccn / xdf

BSD 2-Clause "Simplified" License
87 stars 34 forks source link

Implement sync_timestamps #28

Closed agricolab closed 5 years ago

agricolab commented 5 years ago

Allows to resample (using interpolation for numeric values and shifting for strings) all streams to have their timestamps sample-wise in sync with the fastest stream. Example streams, fileheader = load_xdf(filename, sync_timestamps='linear') This might help adress https://github.com/sccn/xdf/issues/24 and https://github.com/mne-tools/mne-python/issues/5180

cboulay commented 5 years ago

This all looks good. I'm sorry I haven't had time to test. It's on my TODO list.

I'd prefer if pyxdf didn't have a strict dependency on scipy for a single rarely-used function. Can you put the scipy import inside the function that needs it? I'm OK with the occasional user encountering an import error. I'd wager that most users that want to use the interpolation function will already have scipy installed and then for everyone else there's one less dependency.

Or, if there's an argument to be made for including scipy as a dependency in setup.py, then I'm all ears.

agricolab commented 5 years ago

Hey, no sweat. Thanks for taking the time at all.

I agree that less dependencies is better. At first, i just moved scipy into _sync_timestamps. Then i thought, numpy is already a hard dependency for pyxdf, and supports a linear interpolation method. I therefore decided to outsource interpolation to a subfunction, which uses scipy if installed, and attempts to fall back to numpy, if not. I run the tests in an environment without scipy, and they run fine.

Obviously, the latter approach is a little bit more complex, so feel free to stick with the simple https://github.com/sccn/xdf/pull/28/commits/2a606478bfc91e3ea7a34393c2aa8cc06ce383a4 instead

AdeuAndreu commented 5 years ago

Hello guys, I followed ths thread cause i was trying to synchronize two different LSL streams of data. I am not using xdf, though. I checked your implementation @agricolab and I realised that i was doing the interpolation the same way so i wanted to give you my feedback on something i though of doing differently. I noticed that you were using 'linear' interpolation by default.

I think the right interpolation should be 'nearest' and not linear. Because linear will modify the signal as it finds middle points between timepoints that do not exist in the interpolated signal. This might not look a big deal in temporal domain but in frequency domain it could have a different representation.

I attached two pictures of the same signal interpolated with linear and nearest methods: interpolation_linear interpolation_nearest

I hope it could be useful! Cheers!

agricolab commented 5 years ago

Hello guys, I followed ths thread cause i was trying to synchronize two different LSL streams of data. I am not using xdf, though. I checked your implementation @agricolab and I realised that i was doing the interpolation the same way so i wanted to give you my feedback on something i though of doing differently. I noticed that you were using 'linear' interpolation by default.

I think the right interpolation should be 'nearest' and not linear. Because linear will modify the signal as it finds middle points between timepoints that do not exist in the interpolated signal. This might not look a big deal in temporal domain but in frequency domain it could have a different representation.

I attached two pictures of the same signal interpolated with linear and nearest methods: interpolation_linear interpolation_nearest

I hope it could be useful! Cheers!

Hey @AdeuAndreu , thanks for checking.

Yeah, good analysis, and i fully agree. Interpolation is tricky. I'd say what's the most adequate interpolation method depends a lot on the signal. If the sampling rates of the two streams are similar, i 'd agree that "nearest" makes more sense. If one has a very low sampling rate compared to the fastest, "nearest" interpolation can introduce edges, and linear might be superior. Cubic interpolation is smoother, but is not convex and does not preserve monotonocity, etc. etc... In the end, that was why i wanted to allow to control the kind of interpolation with arguments.

I am open to changing the default, though. Currently linear is good as it can be done with numpy only. But probably sth like https://docs.scipy.org/doc/scipy/reference/generated/scipy.interpolate.PchipInterpolator.html#scipy.interpolate.PchipInterpolator is a good default. Maybe i'll find the time to add a monotonic cubic spline and/or more options

Maybe we should also expand the documentation or add a discussion to the wiki.

cboulay commented 5 years ago

@agricolab Do you mind moving this PR to https://github.com/xdf-modules/xdf-Python? (Unfortunately I can't transfer issues between organizations)

agricolab commented 5 years ago

Is there a way to transfer between repositories? xdf-python does not show as option when changing the base... Googling didn't help either, it seems i have to clone xdf-python, add changes and create a new pull request. That will lose the comments on this pull request, but i guess i can link to them in the new. So, unless you have a better idea, will do asap.

cboulay commented 5 years ago

Unfortunately we can’t transfer issues or PRs across organizations. Linking to the old PR is the best we can do.

agricolab commented 5 years ago

Done in https://github.com/xdf-modules/xdf-Python/pull/1