BlackrockRawIO: extend gap_tolerance_ms to the standard nsx formats#1880
Draft
h-mayorquin wants to merge 10 commits into
Draft
BlackrockRawIO: extend gap_tolerance_ms to the standard nsx formats#1880h-mayorquin wants to merge 10 commits into
gap_tolerance_ms to the standard nsx formats#1880h-mayorquin wants to merge 10 commits into
Conversation
Take master's blackrockrawio.py wholesale. The branch's own buffer API migration is superseded by NeuralEnsemble#1820, which merged after review. The gap feature is re-applied on top of master's version in the following commit.
Master detects gaps only for PTP files, where every sample carries its own timestamp. Standard-format files were segmented by making each data block a segment, with no gap detection and no way to merge blocks that a pause never really separated. Split the segmentation into a dispatcher plus one segmenter per format, and give the standard path block-level gap detection through the same _classify_gaps step the PTP path uses. Both hand it the interval between consecutive samples, so the two paths share one threshold and one report. Merging blocks means a segment can now span several of them, at different file offsets, so segments carry a list of memmap specs rather than a single one and _read_multi_block_chunk stitches reads that straddle a boundary. Backward jumps now split as well. A clock reset makes the timestamps run backwards rather than leaving a gap, so there is no duration to compare against the tolerance and the blocks either side must never merge.
_format_gap_report derived a gap's position from a per-sample timestamp array, asking that array to be both the time of the sample before each gap and, via its first element, the start of the recording. Per-sample timestamps satisfy both, so the PTP path it was written for was correct. The standard path has no such array: its file stores one timestamp per block. It passed block timestamps, which got the origin right but not the position, so a gap was reported at the start of the block preceding it rather than at the sample it follows. In pause_correct that put a gap 4 s into the recording at 0.000000 s. Pass the resolved quantities instead, so the report renders what it is given and assumes nothing about how a format stores its timing. The standard path works out the sample before each gap from its block scalars, which is exact because within a block the timing is uniform by definition, and avoids synthesizing per-sample timestamps that would run to gigabytes on a long recording. Both paths now report in the same sample-level terms, which keeps the report reusable as the gap API reaches other readers (issue NeuralEnsemble#1773). Also only build the report when about to raise, rather than on every load.
An empty range touched no block, leaving nothing to concatenate and raising ValueError, where an unmerged segment returns an empty array. Filling a preallocated output gives the empty range a correctly shaped result and keeps merged and unmerged segments behaving alike. The blocks are also no longer all held at once. With a channel list each block is a copy rather than a view, so concatenating held every one of them; now each is written into the output and released. A range inside a single block is still returned as a view, as before.
_get_blackrock_timestamps reconstructed non-PTP times as t_start + index/rate. That was correct only because segmentation gave every data block its own segment, and within a block the sampling is uniform by definition. Nothing stated that dependency. Merging blocks under gap_tolerance_ms removes it. A merged segment has one t_start and a pause somewhere inside it, so the arithmetic walks straight through the pause and reports every later sample early by its length. The accessor returned 4.0 s for a sample pause_correct timestamps at 31.0087 s, without a line of it changing. Keep each contributing block's timestamp on the segment and interpolate per block, which is where uniform spacing is a fact of the format rather than an artifact of the merge. gap_tolerance_ms decides how samples are grouped into segments; it does not decide what time a sample was recorded at. The segment's own t_start and t_stop were always right, being the first and last sample, and stay so. What a merged segment cannot express is the pause in between, which is exactly why these timestamps are worth reporting separately.
…ap report Raising by default earns its place when the reader cannot decide for the caller. A forward gap qualifies: merging across one distorts the segment's uniform time base, splitting gives more segments, and only the caller knows which is acceptable. A backward jump does not. It has no duration to compare against gap_tolerance_ms, so it always becomes a segment boundary and there is nothing to choose. Raising asked a question with one possible answer, and pointed the caller at a parameter that has no effect on it: whatever value they supplied, the answer was the same. Warn instead, naming what was found and what was done about it, and leave backward jumps out of the gap report, which is about gaps. The error now reports only what gap_tolerance_ms can act on, so its advice is true of everything in it. This narrows the breaking change to files with real forward gaps. A file whose only discontinuity is a clock reset loads as it did before, into the same segments, and test_segment_detection_reset goes back to master unchanged. The nev path already warns about resets on these files, so the signal path now says the same thing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Standard-format Blackrock files can arrive split into a large number of segments. A standard file stores a separate data block wherever the recording was not contiguous, and the reader turned each block into its own segment unconditionally, with
gap_tolerance_msaccepted but silently ignored. SpikeInterface #2306 is a spec 2.3 recording that loads as 280 blocks in its ns2 and 387 in its ns5, so the fragmentation is real; what that report does not give is the gap sizes, so I cannot say whether its blocks are close enough that merging is wanted or far enough apart that they are genuine boundaries. This extendsgap_tolerance_ms(already working for PTP (Precision Time Protocol) files) to the standard formats so the caller decides: gaps are detected between block timestamps, the reader raises by default with a report,gap_tolerance_ms=0keeps every block as its own segment (the previous behaviour), and a larger value merges blocks whose gap is below it.Clock restarts are handled separately from gaps. A reversal, where the timestamps run backwards between consecutive samples, has no duration to weigh against a tolerance, so it always starts a new segment and warns rather than raising. This matches what the nev path has done for specs 2.3 and 3.0 since reset support landed, and it is what stops
ResetCorrectmerging across its clock restart under a large tolerance.Merging breaks the within-segment uniform-timing assumption. A segment carries a single
t_start, sot_start + index / sampling_rateplaces samples after a merged gap earlier than they were recorded, by the gap's length. That uniform-sampling assumption runs throughout Neo and stops being true once a gap is tolerated, which is why_get_blackrock_timestampsreports the recorded times regardless of the tolerance: grouping into segments is policy, the timestamps are fact, and the default raises rather than silently handing back the shifted view.It is a breaking change, deliberately not branched by spec. A multi-block standard file that used to load now raises unless a tolerance is passed. Error-by-default already shipped for PTP (Precision Time Protocol) files in Fix over-segmentation in BlackRock #1789; rather than give the standard formats a softer default, they inherit the same contract, so
gap_tolerance_ms=Nonemeans the same thing for every Blackrock variant instead of depending on which one was opened.