Stop doing full-array work at open to answer approximate questions - #23
Merged
cboulay merged 2 commits intoAug 29, 2026
Merged
Conversation
NWBSlicer construction on a 5-stream 5.7 GB recording: 218 ms -> 119 ms with the dejitter cache warm, 250 -> 119 cold. Two thirds of what is left is inside pynwb's read(), building Python objects for the file, which is not ours to fix cheaply. np.median over every interval, 45 ms per stream. The median here is never the answer -- infer_nominal_rate returns a mean over the intervals that survive a trim window, and the median only says where to centre that window. Partitioning 7 million floats to place a window is precision nobody asked for. It is not even approximate in practice. Timestamps arrive on a quantisation grid, so the median snaps to a grid point that a stride cannot move: strides of 16 through 1024 over a real 7.09M-interval stream all give 3.3301000002e-05. Verified end to end that every stream in that recording reports a bit-identical rate with the subsample as with the full median, including the two _device_ts streams that actually reach the inference path at 30137.773554857125 and 30000.082189805245. Strided rather than head-of-array, so a stream whose intervals drift between its start and its end is represented across the estimate instead of by whichever end happened to be read first. np.var was also computed twice to evaluate one `or`. Short-circuiting meant the second pass only ran when the first comparison failed -- which is exactly the irregular-stream case, i.e. the slow one. electrodes.table.to_dataframe(), 12 ms per electrical series. It materializes every column the writer stored -- position, group, filtering -- into pandas so we can read one of them, and it drags in a reference resolution per row on the way. Indexing the label column directly is the same values for a thirtieth of the work. The fallback for a table with no label column now names channels from the id column rather than from a DataFrame index. That is not a change: the index to_dataframe() built was the id column. Making it explicit is what keeps it correct now that the DataFrame is gone, since positional indices coincide with ids only when a series references the whole table in order. Tests pin equivalence rather than speed: _fast_median against np.median across the threshold, inferred rates unchanged, and -- because the trim is the only reason a mean is usable here -- that a stream with 400 dropped-packet holes still reports 30 kHz rather than an average including the holes.
Cold open -- the first time a file is seen, which for batch analysis is every file -- spent ~0.67 s taking medians over full recordings inside clockmodel.py, which the earlier slicer-side fix did not reach. Reconstruction time for a 5-stream 5.7 GB recording: 1.02 s -> 0.57 s, measured by alternating the two implementations so drift hits both. _fast_median moves to util.py as interval_median, since clockmodel cannot import from slicer (slicer imports clockmodel) and util imports neither. The rename says what the shortcut is licensed by: these are medians of inter-sample intervals, and intervals inherit the device's quantisation grid, so the median snaps to a grid point a stride cannot move. Verified on a real recording that all five streams reconstruct bit-identical timestamps and the gap threshold matches to the last digit. _auto_gap_threshold takes the shortcut for its median and deliberately not for its percentile. Subsampling the whole function is 16x faster and moves the threshold 1.4%, because a 99.9th percentile over 200k values rests on their top 200 instead of on 7000. That threshold decides which jumps are real gaps, so moving it re-segments the stream and re-times its samples -- a behaviour change wearing a speedup's clothes. A test asserts the asymmetry so nobody 'finishes the job' later. through_c's median is left alone for the same reason: it averages fit residuals, which are continuous and have no grid to snap to.
cboulay
force-pushed
the
perf/faster-slicer-init
branch
from
August 29, 2026 06:09
1620b7d to
17ac757
Compare
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
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.
Stacked on #22 — review that first, and this diff is just the two commits on top.
Opening an
NWBSlicerdid several full passes over a recording's timestamps to compute statistics that never needed that precision. On a 5-stream 5.7 GB recording:The cold number is the one that matters for batch analysis, where every file is seen for the first time.
What was slow
np.medianover every inter-sample interval — 45 ms a go, several times per stream.infer_nominal_rateuses the median only to centre a trim window; the estimate it returns is a mean over the intervals that survive. Partitioning 7 million floats to place a window is precision nobody asked for.clockmodel's_nominal_periodand_auto_gap_thresholddo the same thing on the same arrays.np.varcomputed twice to evaluate oneor. Short-circuiting meant the second full pass only ran when the first comparison failed — which is exactly the irregular-stream case, i.e. the slow one.electrodes.table.to_dataframe()— 12 ms per electrical series. It materialises every column the writer stored (position, group, filtering, …) into pandas, and resolves an object reference per row, in order to read one column.Why subsampling the median is exact here
Timestamps arrive on a device's quantisation grid, so their differences cluster onto that grid and the median snaps to a grid point that a stride cannot move. On a real 7.09M-interval stream, strides of 16 through 1024 all return
3.3301000002e-05, andinfer_nominal_ratereturns30147.9172from every one of them.Verified end to end on that recording: all five streams reconstruct bit-identical dejittered timestamps, and the gap threshold matches to the last digit. Two of those streams reach the inference path with non-trivial rates (
30137.773554857125and30000.082189805245), so this isn't only exercising the easy cases.The helper is named
interval_medianand lives inutil.py, which imports nothing local —clockmodelcan't import fromslicer, sinceslicerimportsclockmodel. The name states what licenses the shortcut: intervals inherit a grid, and other quantities don't.What deliberately did not change
_auto_gap_thresholdkeeps its percentile on the full array. Subsampling the whole function is 16x faster and moves the threshold 1.4% (0.013372→0.013557), because a 99.9th percentile over 200k values rests on their top 200 instead of on 7,000. That threshold decides which interval jumps count as real gaps, so moving it re-segments the stream and re-times its samples — a behaviour change wearing a speedup's clothes. A test asserts the asymmetry so nobody finishes the job later.through_c's median is untouched for the same reason: it averages fit residuals, which are continuous and have no grid to snap to.Tests
tests/test_slicer_perf.pypins equivalence, not speed —interval_medianagainstnp.medianacross the threshold,_nominal_periodand_auto_gap_thresholdagainst themselves computed with a full median, and inferred rates unchanged. Because the trim is the only thing that makes a mean usable here, one test gives a stream 400 dropped-packet holes and asserts it still reports 30 kHz rather than an average including the holes. Another pins that thelabelcolumn read honours the electrodes region, since a series may reference a subset of the table.The fallback for a table with no
labelcolumn now names channels from theidcolumn rather than a DataFrame index. That is not a change — the indexto_dataframe()built was theidcolumn — but it has to be explicit now that the DataFrame is gone, because positional indices coincide with ids only when a series references the whole table in order.256 tests pass; ruff clean.
What's left, for the record
After this, opening is ~119 ms, of which 108 ms is inside
pynwb.read()building Python objects. About 24 ms of that is pynwb#2253 (submitted upstream):FeatureExtraction.__init__copiestimesinto a Python list, reading the backing dataset one element at a time — 23,630 separateh5pyreads on this file, 84x slower than one bulk read. The rest is hdmf's object construction and not ours to fix cheaply.Chunk iteration was profiled separately and needs no work: per-chunk CPU in our code is ~0.07 ms against an 11–16 ms chunk, and raw
h5pyforward reads run at the same 1.09 GB/s the iterator achieves. It is disk-bound, not us.🤖 Generated with Claude Code