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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jfrey-xx
Copy link

@jfrey-xx jfrey-xx commented Dec 1, 2017

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.

Fix crash when used with eeglab associataed functions.
@jfrey-xx jfrey-xx changed the title Compute effective sampling rate even when nominal rate is 0 Compute effective sampling rate even when nominal rate is 0 + better handle empty streams. Dec 1, 2017
@cboulay
Copy link
Collaborator

cboulay commented Dec 1, 2017

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.

@jfrey-xx
Copy link
Author

jfrey-xx commented Dec 1, 2017

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)?

@cboulay
Copy link
Collaborator

cboulay commented Dec 1, 2017

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.

@jfrey-xx
Copy link
Author

jfrey-xx commented Dec 1, 2017

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

@jfrey-xx
Copy link
Author

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?

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

Successfully merging this pull request may close these issues.

2 participants