xdf icon indicating copy to clipboard operation
xdf copied to clipboard

Compute effective sampling rate even when nominal rate is 0 + better handle empty streams.

Open jfrey-xx opened this issue 7 years ago • 5 comments

Hello,

When I was dealing with LSL streams that were created with a sampling rate set to 0 (and that could make sense, for instance with https://github.com/xfleckx/LSL4Unity since the framerate could be erratic) I got errors once I tried to import in eeglab with eeg_load_xdf.m. Also it crashed with empty streams, hence this fix (bonus: some variables renamed to make them more consistant across files).

At the time I did not think about writing-down all details related to the errors, but if you need to better review the code I can try to reproduce the problem.

jfrey-xx avatar Dec 01 '17 17:12 jfrey-xx

It seems simple enough, but.. What are the downstream functions relying on effective_srate? I'm worried they might be doing things that assume a constant sample interval (e.g., filtering). If we provide a fake effective_srate then we may be allowing incorrect processing to occur which is IMO worse than no processing.

cboulay avatar Dec 01 '17 17:12 cboulay

I see your point. "effective" sampling rate, but not a constant one. To prevent this false assumption, downstream functions could maybe check that the nominal sampling rate is 0 (i.e. "not constant") and then the effective sampling rate is just a convenient computation that they should not take into account (and/or issue a warning for the user)?

jfrey-xx avatar Dec 01 '17 18:12 jfrey-xx

eeg_load_xdf already has an option to use effective_rate instead of nominal_rate, and that option defaults to False, so that's a pretty good start.

How about,

  1. In load_xdf.m, print a warning when nominal_rate is 0 that the calculated effective_rate might not be meaningful and relying on this rate is not recommended.
  2. In eeg_load_xdf.m, in the parameter description in the help text block, add a sentence that says using effective_rate can lead to incorrect results.

I think those two warnings together should be sufficient to scare people off if they don't know the implications of what they are doing. If they know what they are doing then we shouldn't stop them.

cboulay avatar Dec 01 '17 18:12 cboulay

Seems perfectly fine to me as well, here is a proposal.

jfrey-xx avatar Dec 01 '17 19:12 jfrey-xx

Hello,

New batches of experiment will start soon in my lab, that would help if these commits are integrated upstream. Is there something I can do to ease the merge?

jfrey-xx avatar Dec 10 '17 13:12 jfrey-xx