Handling flags and time offset (-ftime option) for >1000 Hz data

[flyingfalling]

I noticed that the current version has no mechanism for returning appropriate floating point time for sample rates >1000 Hz. Currently it provides the (edf2asc etc.) default of simply doubling up "time", without adding the 0.5 offset that is specified by the "SAMPLE_ADD_OFFSET" bit (SAMPLE_ADD_OFFSET = 0x0002 # if this flag is set for the sample add .5ms to the sample time), see edf_data.pyx.

When running edf2asc for example, the "-ftime" flag causes 0.5 msec to be added to time for every sample with the SAMPLE_ADD_OFFSET flag set (which is for every other sample when recorded at 2000 Hz). And time is returned as a floating point number rather than large integer.

A slightly separate issue: the 'flags' column is returned in the samples dataframe as a floating point column instead of the (16-bit) integer flags it should (and it is shadowing pandas.DataFrame.flags...making things confusing to the casual coder accessing via df.flags attr instead of df['flags']...).

I'm trying to work on a fix for this (and will require adding a default-off option argument in read_edf).

Thoughts?

[flyingfalling]

To reproduce the "issue", run read_edf on any data recorded at 2000 Hz. Samples will be doubled up in "time", i.e. every pair of adjacent rows will have the same time value. We could "hack" this easily and simply add 0.5 to the second row of every pair, but a "safer" way to do it (in case EDF data is out of order?) is to check the flag for the bit and act accordingly.

[flyingfalling]

OK, here is an example function to compute the correct times (only where necessary) using the flags. It also adds an "iflags" (int16) column in case users want to use the flags downstream.

I will add a pull request that runs this on the "samples" before returning them for read_edf, if an option "ftime" is set to true (default false I guess?)

def fix_edf_sampletimings(s):
    import numpy as np;
    import pandas as pd;
    bitmask=pyedfread.edf_read.SAMPLE_ADD_OFFSET; #bitmask = 0x0002;              
    HALFMS=0.5; #unit is already MSEC for edf's "time".                  
    s = s.copy();
    if( False == pd.api.types.is_float_dtype(s.time) ):
        s['time'] = s['time'].astype(np.float64);
        pass;
    s['iflags'] = s['flags'].round().astype(np.uint16);
    s['halfms'] = (0 != (bitmask & s['iflags']));
    uncorrected = s.time - s.time.round(decimals=0);
    FP_DELTA=0.1;

    ## true/false mask for rows whose time values are near whole numbers 
    ## i.e. those which are possibly uncorrected yet.                    
    uncorrected = ((uncorrected > -FP_DELTA) &
                   (uncorrected < FP_DELTA));

    ## Only modify those that are possibly uncorrected yet, and also     
    ## have the flag to add +0.5 set. Add HALFMS each.                   
    s.loc[ ((True==s['halfms']) & (uncorrected)), 'time' ] += HALFMS;
    return s;

[behinger]

🙈 Thanks! I actually stumbled into this myself before becoming maintainer of pyedfread. I didnt remember to fix it upstreams. I had just recalculated the timestamps after everything.

• I dont think EDF data can be out of order - at least I think I remember reading this sometime, but if I understand your solution correctly, then this is taken care of at the cost of a n-sample memory allocation. Should be fine
• is it really necessary to make a copy? I don't have the context in my mind right now
• I dont understand how this line uncorrected = s.time - s.time.round(decimals=0); can return a 1 if s.time was an int casted to float. Is this in cases where time is already float? when can this happen?

I like that you mimick the ftime of edf2asc; I would go a step further and automatically activate it, in case it is not specified & >1000 Hz detected. I cannot imagine a scenario where you dont want to have this; but I can imagine most users would want to have the half ms step.

I would also add a further notice, that the EDF-precalculated event-timings are calculated on the non-float time, thus always will be locked to the first timestamp.

Super cool, thanks for improving this tool!

[flyingfalling]

Hey, nice that you solved it before!

Regarding out-of-order, I see it quite often, but I am figuring it out why. It seems it may be some kind of issue with my wrappers. Actually, it is not "out-of-order" real data, it is due to random noise in read EDFs, which I am currently grappling with debugging, so it may not be necessary. I'm not sure if it is due to reading un-zeroed memory or what.

That is actually what the "copy" is for...you don't know if the passed DF is a slice or what, so copying it makes it clear that you should treat it as a new DF with contiguous memory...at least that was my intention. Of course we can remove it if it never causes bugs. The function I wrote was supposed to be general, not a part of EDFread. In our case, we construct the DF so we of course know that it should be fine without a copy(). Copying is very cheap (probably CoW -- yea looks like depending on PANDAS version it should be COW - https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.copy.html)

Time is already a float when returned in the pandas dataframe unfortunately... (at least on my side?) Do you get integers?! Every single column is floating point by default. That is why I do the weird finangling with rounding, because due to imperfect floating point representations, unfortunately casting to int will not always give you the correct number...neither will simple rounding or floor/etc, nor adding some delta. Maybe I need to check the backend libraries better though, in case python does something magical that C doesn't with casting?

Yes, I was worried the events DF would also need some correction, but after checking with SR research, they are always locked to whole-milliseconds, so there is no issue there.

I agree it could be on by default, but honestly since the EDF library doesn't do that, it might be a bad idea to break that compatibility. Just putting it as an obvious option is best I think? But, you can decide that based on your preference. Either way works for me. I was trying to not break backwards compatibility.

[flyingfalling]

Yes, I am definitely hitting a repeated but random issue in which pyedfread continues to "read" past the end of the EDF file samples. I'm hoping it is a mis-sized buffer or something because I really don't want to mess around with these pyx files. Maybe it has only to do with 2k hz files? Have you never seen this issue?

[flyingfalling]

EDIT: SOLVED -- it was a mis-sized samples array which is only partially used but the whole thing is returned.

It is definitely reading past the number of samples in EDF files. I will investigate what it is in pyedfread source.

[behinger]

I have not seen this yet