BiocamRawIO: reshape flat datasets instead of gathering channel by channel - #1893
Open
adityasingh2400 wants to merge 1 commit into
Open
BiocamRawIO: reshape flat datasets instead of gathering channel by channel#1893adityasingh2400 wants to merge 1 commit into
adityasingh2400 wants to merge 1 commit into
Conversation
…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
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.
BiocamRawIO._get_analogsignal_chunkhas two branches. Older files hand back a 2D(n_samples, n_channels)array and are indexed directly. Newer files (BRW 4.xWell_*/Raw, and the v3101/102formats) hand back a flat frame-major 1D array, and that branch first expands the caller's selection into a list withrange(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, becauseortreats0as absent andrangehas 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, andslice(-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 costsn_channelscache-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 raisesNeoReadWriteErrornaming 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) andbiocam_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 forNone,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. Fullrawio_compliancepasses on both, as does aBiocamIO.read_block(). The new tests build a minimal BRW 4.x file with h5py in the style of the existingtest_biocamrawio_gain, so they need no downloaded data. Against 35cbce7 they are 5 failed 9 passed before the source change and 14 passed after, andneo/test/rawiotestplusneo/test/coretestis 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_sparsereturns a 2D array and so has always taken the other branch.Fixes #1892