Skip to content

BiocamRawIO: reshape flat datasets instead of gathering channel by channel - #1893

Closed
adityasingh2400 wants to merge 1 commit into
NeuralEnsemble:masterfrom
adityasingh2400:fix-1892-biocam-flat-reshape
Closed

BiocamRawIO: reshape flat datasets instead of gathering channel by channel#1893
adityasingh2400 wants to merge 1 commit into
NeuralEnsemble:masterfrom
adityasingh2400:fix-1892-biocam-flat-reshape

Conversation

@adityasingh2400

Copy link
Copy Markdown

BiocamRawIO._get_analogsignal_chunk has two branches. Older files hand back a 2D (n_samples, n_channels) array and are indexed directly. Newer files (BRW 4.x Well_*/Raw, and the v3 101/102 formats) hand back a flat frame-major 1D array, and that branch first expands the caller's selection into a list with range(start or 0, stop or n_ch, step or 1), then rebuilds the chunk with one strided gather per channel.

The range(...) expansion is wrong for any slice carrying a zero or negative bound, because or treats 0 as absent and range has no notion of negative indices. On a 16 channel file, slice(0, -1) returns 0 channels instead of 15, slice(None, None, -1) returns 0 instead of 16, slice(0, 0) returns all 16 instead of none, and slice(-2, None) returns 18 columns, which is more columns than the file has channels, with the first two silently filled from the wrong end of the flat buffer. None of these raise. Every one of them is correct today on an older 2D file, so the same reader answers the same question two different ways depending on which firmware wrote the recording.

The per-channel loop is also the reason issue #1892 was filed. Its comment says it avoids loading all channels into memory, but self._read_function(...) on the line above has already read the whole span into a numpy array, so the loop buys no memory and costs n_channels cache-hostile passes plus a second full-size allocation for the destination.

The fix reshapes the flat array to (i_stop - i_start, n_channels), which is a view onto the buffer that was just read, and then lets both layouts share one indexing expression. numpy applies the caller's slice, index list, or array exactly as it does everywhere else in Neo, so the slice forms above stop disagreeing with the 2D branch. A frame range that runs past the end of the dataset used to surface as a numpy broadcast error from inside the loop, and now raises NeoReadWriteError naming the range that was asked for.

Verified on the two BioCam files in the gin test data, biocam_hw3.0_fw1.6.brw (36 channels, readHDF5t_101) and biocam_hw3.0_fw1.7.0.12_raw.brw (100 channels, readHDF5t_brw4). Against a verbatim copy of the old branch the output is bit identical for None, slice(None), slice(0, 4), slice(2, None), slice(None, None, 3) and an explicit index list, and reading 2000 frames is 9x faster on the first file and 68x on the second. Full rawio_compliance passes on both, as does a BiocamIO.read_block(). The new tests build a minimal BRW 4.x file with h5py in the style of the existing test_biocamrawio_gain, so they need no downloaded data. Against 35cbce7 they are 5 failed 9 passed before the source change and 14 passed after, and neo/test/rawiotest plus neo/test/coretest is 637 passed 130 skipped 0 failed with the change in place.

I could not test the event-based sparse path against a real compressed recording, but that path is untouched: readHDF5t_brw4_sparse returns a 2D array and so has always taken the other branch.

Fixes #1892

…annel

The flat (frame-major) branch of _get_analogsignal_chunk expanded a slice
with range(start or 0, stop or n_ch, step or 1), which silently mishandles
any slice carrying a zero or negative bound, and then rebuilt the chunk with
one strided gather per channel over an array that _read_function had already
read fully into memory.

Reshaping the flat array to (n_samples, n_channels) makes both file layouts
share the same indexing expression, so numpy applies the caller's slice or
index list directly and every slice form behaves as it does everywhere else
in Neo. A frame range that runs past the end of the dataset now raises
NeoReadWriteError naming the range rather than a numpy broadcast error.

Fixes NeuralEnsemble#1892
@zm711

zm711 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

#1885 works to address this. I would suggest testing it out first to see if that fixes your performance issue as we are about to merge that PR.

@adityasingh2400

Copy link
Copy Markdown
Author

Checked it, and yes, #1885 fixes this. I think it should land instead of mine, so I am closing this.

It carries the same core change, dropping the per-channel loop for a reshape onto the flat buffer and a plain slice:

data = data.reshape(i_stop - i_start, self._num_channels)

It is also the better version. The loop existed to keep memory bounded when only a few channels are wanted from a large recording, which is SpikeInterface#3303. My PR removes the loop and gives that bound up. #1885 removes it too but replaces it with an explicit ceiling, max_read_bytes_dense and max_read_bytes_sparse, and splits the read when the buffer would be large and the output small. That keeps the property I dropped, and the separate sparse ceiling is a detail I had not considered, since the event decoder re-walks its tables per call so it wants fewer, bigger reads.

One small thing you may or may not want from mine. I added a guard that raises when the flat read comes back the wrong length, which turns an out of bounds frame range into a clear error rather than a reshape traceback:

expected_size = (i_stop - i_start) * self._num_channels
if data.size != expected_size:
    raise NeoReadWriteError(...)

Happy to send that as a small separate PR against #1885 once it merges if you think it is worth having. Otherwise no need, and thanks for pointing me at the right PR.

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.

BiocamRawIO: per-channel loop over already-loaded data makes BRW4 reads ~18x slower than necessary

2 participants