Add ignore_timestamps_errors argument to OpenEphysRawIO
#1213
Conversation
|
Is it ok if we relax the assertion to a warning in this case? See #1210 I can change the failing test accordingly. |
|
Hi @alejoe91 I think a warning might be too relaxed in this case as even for significant pauses this might be missed by the user. What the deviation of timestamps you observed in the test files provided by @ThePuppetShow? Maybe we can make a reasonable estimate of what deviations are acceptable and should only raise a warning? |
|
If I remember correctly, there were a bunch of zeros at the beginning of the timestamps file, but we might need to double check. On the other hand, the assertion would completely prevent from loading the data in case something is wrong with timestamps. I think that the API should allow to at least load the data, even just to check what is wrong and maybe cut out "good" portions of the recording. No? |
|
Hey, and thanks a lot for looking into this I got this issue in some but not all my recordings. To my knowledge none of the recordings should not have started with any pauses, so I'm not sure why sometimes the timestamps were not directly from 0. Maybe a lag of sorts? In one of these sessions the first timestamp was at 1.536 s. I made some plots here #1210 Would it be possible to at least allow for the data to be accessed, but give a warning? Then I can just adjust it myself, if the timestamps would get mismatched somehow. Or just automatically scale it if it starts with a slight delay? Again, thanks a lot for taking the time to look into this : ) Best |
|
Hi @alejoe91 I would argue that small deviations could pass with just a warning, but for bigger deviations an error should be raised. For debugging purposes I would propose to add a keyword to the IO that permits to ignore that error and still return the data (with wrong timestamps). This way the user would need to actively decide to load incorrect timestamps and it is not happening silently. |
|
That's fair! :) Something like |
alejoe91
changed the title
ignore_timestamps_errors argument to OpenEphysRawIO
|
@JuliaSprenger should be done |
There was a problem hiding this comment.
Hi @alejoe91 thanks for implementing the new flag.
I noticed that until now this was raising a plain Exception and not a more specific Error. Do you agree this error should be more specific?
| all_samplerate.append(chan_info['sampleRate']) | ||
|
|
||
| # check for continuity (no gaps) | ||
| diff = np.diff(data_chan['timestamp']) |
There was a problem hiding this comment.
We have to keep in mind that this diff have a very high cost when opening.
on spiekinterface side this means when parallel processing making n_jobs time this check!!!
There was a problem hiding this comment.
This is legacy OE..we can fix it in a different PR of needed
There was a problem hiding this comment.
@samuelgarcia Feel free to make a proposal on this in a separate PR.
|
Failing tests are due to #1227 and not related to this PR. |
5 participants
fixes #1210