Fix the Units waveforms test fixtures and assert the doubly indexed structure - #2240
Open
adityasingh2400 wants to merge 2 commits into
Open
Conversation
…tructure TestUnitsIO and TestUnitsWaveformsOnlyIO labelled the outer dimension of the 3-D waveforms input to add_unit as electrodes and the middle dimension as spike events. add_unit does the opposite: dim 0 becomes waveforms_index_index (spike events per unit) and dim 1 becomes waveforms_index (waveforms, one per electrode, per spike event), which matches the schema description of the doubly indexed waveforms column. Because of the swapped labelling the fixtures also contradicted their own spike_times. TestUnitsIO declared 3 spike_times per unit but encoded 2 and 3 spike events, and TestUnitsWaveformsOnlyIO declared 3 spike_times per unit but encoded 2 spike events. Neither test asserted anything about waveforms, so this was never caught. Reorder both fixtures to (num_spikes, num_electrodes, num_samples), give TestUnitsIO distinct sample values so a transposition is detectable, and add test_waveforms_structure asserting waveforms_index_index, waveforms_index, and the 2-D waveforms dataset after a roundtrip.
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.
Motivation
Addresses the test-fixture half of #2220. I have deliberately left
misc.pyand thewaveforms_descdocstring alone so this does not conflict with #1936, which is already touching that text.For the doubly indexed
Units.waveformscolumn,add_unitinterprets a 3-D per-unit input as(num_spikes, num_electrodes, num_samples). Dim 0 becomeswaveforms_index_index, the spike events of a unit, dim 1 becomeswaveforms_index, the waveforms of a spike event with one per electrode, and dim 2 is the samples of each waveform. That matches the schema, which says thewaveforms_indexcolumn "indexes which waveforms in this column belong to the same spike event for a given unit, where each waveform was recorded from a different electrode" and thewaveforms_index_indexcolumn "indexes thewaveforms_indexcolumn to indicate which spike events belong to a given unit".Both
TestUnitsIOandTestUnitsWaveformsOnlyIOlabel these the other way round, with the outer dimension commented as electrodes and the middle one as spikes. Because of that the fixtures also contradict their ownspike_times:TestUnitsIOdeclares 3spike_timesfor each unit but encodes 2 spike events for the first unit and 3 for the second, andTestUnitsWaveformsOnlyIOdeclares 3spike_timesper unit but encodes 2 spike events. Neither test asserted anything about the waveforms columns, so nothing caught it.Changes
Both fixtures are reordered to
(num_spikes, num_electrodes, num_samples)so the number of spike events matches the number ofspike_times, and the comments now describe whatadd_unitactually does.TestUnitsIOgets distinct sample values,1through54, instead of the repeated[1, 2, 3]rows, so a transposed or mis-flattened write is detectable rather than looking identical.A new
test_waveforms_structureroundtrips the container and assertswaveforms_index_index,waveforms_index, and the 2-Dwaveformsdataset, plus the per-spike electrode counts. This is the first coverage of the doubly ragged structure of this column.How to test the behavior?
Running the new assertions against the old fixture shows the inconsistency directly. The old first unit had 3
spike_timesbut only 2 spike events:With the corrected fixtures:
The equivalent minimal repro of the ordering, on
devand unchanged by this PR:Checklist
ruff check . && codespellfrom the source directory.