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

pop_mergeset places boundary triggers at fractional latencies #95

Closed
nucleuscub opened this issue Dec 3, 2019 · 3 comments
Closed

pop_mergeset places boundary triggers at fractional latencies #95

nucleuscub opened this issue Dec 3, 2019 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@nucleuscub
Copy link
Member

nucleuscub commented Dec 3, 2019

Bug report 1374 in BUGZILLA

[reply] [−]Description Andreas Widmann 2012-11-20 02:03:46 PST
Boundary/discontinuity triggers should always mark the first sample of a new epoch to avoid unexpected non-linearities. pop_mergeset (and possibly also other functions) place the boundary trigger at .5 between merged datasets. Many common operations affecting latency or sampling rate may give unexpected results. E.g. downsampling the merged data to half the sampling rate moves the boundary event to a .25 latency. In consequence the boundary detection algorithm in pop_eegfilt will incorrectly shift the boundary event by one sample; filtering across this now incorrectly marked boundary results in a (often large) DC artifact. The only consistent way to avoid these non-linearities is to mark only full samples with this type of event.

[reply] [−]Comment 1 Arnaud Delorme 2014-06-11 15:51:40 PDT
Has been fixed by Andreas. Arno

[reply] [−]Comment 2 Andreas Widmann 2014-06-12 02:33:24 PDT
No, this has NOT been fixed yet! The only part that has been fixed is shifting of boundary events to other latencies as .5 during resampling. This was the separate bug https://sccn.ucsd.edu/bugzilla/�0�. Andreas

[reply] [−]Comment 3 Ramon Martinez-Cancino 2014-07-02 11:37:47 PDT
Hi Andreas, When resampling, boundary events ae adjusted so their latency remain 0.5. So the filtering mechanism should be OK. Would you mind to look into it? Thanks, Arno

[reply] [−]Comment 4 Andreas Widmann 2014-07-07 02:37:24 PDT
Hi Arno, the major problem in resampling has been fixed, yes. However, I think this is a more fundamental problem. All EEG recording software I have ever used (NeuroScan Acquire, BrainVision Recorder, BioSemi ActiView, Instep) place discontinuity or DC correction markers at the first sample of the new segment. To my understanding this is the expected standard and necessary to avoid non-linearities. Placing of .5-latency boundary markers by EEGLAB might lead to confusion with any new function/new developer. There is for example the problem/bug that ERPLAB filtering only expects .5-latency boundary markers and, thus, ignores or shifts boundaries imported from BrainVision markers (see http://sccn.ucsd.edu/pipermail/eeglablist/2014/007955.html). There is no straightforward and unambiguous solution to compute correct event latency if you have to expect integer and .5 (non-integer) boundary event latencies (you can not trust ceil or round with floats) My suggestion was to change the behavior in EEGLAB in general. That is, identify all functions that set (as e.g., pop_mergeset) or use boundary markers and adjust their behavior to use integer boundary event latencies. I am not sure how many functions would be affected. I think the number would be limited and I would be willing to support some reprogramming. I am aware that this might be a difficult decision. However, I indeed expect issues re-occurring from time to time as in ERPLAB. Best, Andreas

@nucleuscub nucleuscub added the enhancement New feature or request label Dec 3, 2019
@arnodelorme
Copy link
Collaborator

arnodelorme commented Dec 3, 2019

I have added a test case (attached as well) @widmann . All seems OK when resampling in terms of preserving the correct boundary even latency. Moving the boundary event to full samples should be functional without any change (or if we import them in this way) but we should thoroughly test it.

ERPLAB functions could shift all boundary events by +0.5 for added compatibility.

I am reluctant to change this behavior since then I am concerned other parts of EEGLAB will not be working properly. For me it makes sense to place the boundary in between sample since the boundary does not belong to either the preceding or the following sample.
testcase_boundary.m.zip

@widmann
Copy link
Contributor

widmann commented Dec 4, 2019

In all raw data formats I'm aware of the DC offset/new segment/discontinuity markers are on the first sample of the new segment. Thus, simply shifting all boundary events by half a sample will not be going to work. Of course, doesn't make a difference if you ignore signal discontinuities (#73). Anyway, I'm aware that you are reluctant to change. So, I'm reluctant to exchange further arguments and invest any more time.

@arnodelorme
Copy link
Collaborator

Someone should test how EEGLAB behaves when the boundary is on the sample.

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

No branches or pull requests

3 participants