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

openephys legacy format : handle gaps more correctly #1387

Merged
merged 9 commits into from
Feb 2, 2024

Conversation

samuelgarcia
Copy link
Contributor

@samuelgarcia samuelgarcia commented Jan 30, 2024

@weiglszonja @bendichter @alejoe91 @CodyCBakerPhD
This PR should fix the gap problem in openephys legacy format.

The old an erroreneous ignore_timestamps_errors is deprecated.
Now there is a gap checking when parsing the header. If gaps are detected then internally the reader go in a gap mode reading which is slower because it need np.seachrsorted at every get_analogsignal_chunk().

I propagate this on spikeinterface side here SpikeInterface/spikeinterface#2450

@samuelgarcia samuelgarcia added this to the 0.13.0 milestone Jan 30, 2024
@samuelgarcia samuelgarcia marked this pull request as ready for review January 31, 2024 10:30
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple initial assert/error question comments for this.

neo/rawio/openephysrawio.py Outdated Show resolved Hide resolved

if channel_has_gaps:
# protect against strange timestamp block like in file 'OpenEphys_SampleData_3' CH32
assert np.median(diff) == RECORD_SIZE, f"This file has strange block timestamp for channel {chan_id}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange is a bit hard to parse as an end user. What does strange mean? The previous not continuous would make the person think about the fact they paused/stopped and remember. Strange is not quite as descriptive. Is the issue that there are other problems than pauses/stops?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's the file I'm thinking of, the 'strangeness' came from the headstage getting unplugged mid recording

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha. That would make the recording a bit 'strange', but I think the old message still covered that. I can't think of a better single word then strange, but maybe the old verbosity would still be better than a difficult to parse word?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will find a better message. The old message is outdated because we handle gaps now.
But in some case we have assymetric block size series instead of 1024 1024 1024 2048 1024 which is now correct we can have 700 524 700 524 700 524 which is very hard to handle and maybe is wrong.

neo/rawio/openephysrawio.py Outdated Show resolved Hide resolved
@weiglszonja
Copy link
Contributor

Thank you @samuelgarcia, I can confirm this fixes the files that I have from the lab where this issue was discovered. The read time is definitely slow;

reader = OpenEphysRawIO(dirname=folder, ignore_timestamps_errors=None)
reader.parse_header()

6-7 minutes to parse the header and

traces = reader.get_analogsignal_chunk(block_index=0, seg_index=0, i_start=0, i_stop=sig_size, stream_index=stream_index, channel_indexes=None)

and I measured 5 minutes to retrieve the traces for the whole recording (~3 hours) with 32 channels.

I think this is acceptable until we can guarantee that the timestamps are continuous and aligned with the data.

@bendichter
Copy link
Contributor

That sounds good to me!

samuelgarcia and others added 2 commits February 2, 2024 12:04
neo/rawio/openephysrawio.py Outdated Show resolved Hide resolved
@alejoe91 alejoe91 merged commit cd0d75b into NeuralEnsemble:master Feb 2, 2024
1 check passed
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.

6 participants