UnitSelectionSorting improvements: remove lexsort, fix bug, save memory by sharing parent cache when possible - #4783
Open
grahamfindlay wants to merge 2 commits into
Conversation
analyer bug Per SpikeInterface#4606 and SpikeInterface#4618, full lexsort is no longer needed (it is ordered going into remap_unit_indices_in_vector, and that function can't mess up the ordering). This also fixes a bug where `SortingAnalyzer.select_units` with reordered units was incorrectly lexsorting the new sorting's spike vector, permuting cotemporal spikes into unit-ascending order, while extensions sliced its per-spike data (e.g. amplitudes, locations, etc.) with a mask over the old (i.e. unpermuted) spike vector, causing a discrepancy in ordering where row i of the extension data didn't match row i of the new (permuted) spike vector. There were warnings about this already in the code: ``` ``` (although to be clear, the cause of the problem here wasn't on the extension side, but on the Sorting side). `test_selection_preserves_parent_order`and `test_select_units_reordered_keeps_extension_alignment` (sorry for this name, open to suggestions) should hopefully prevent this from happening again.
identity selections (including rename). Can save many GBs of memory. This also keeps a lazy Zarr spike vector lazy (SpikeInterface#4623).
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.
Follows #4606, #4581, #4618, #4695. Just copying my commit messages here:
Commit 1
Per #4606 and #4618, full lexsort is no longer needed (it is ordered
going into remap_unit_indices_in_vector, and that function can't mess upthe ordering).
This also fixes a bug where
SortingAnalyzer.select_unitswithreordered units was incorrectly lexsorting the new sorting's spike
vector, permuting cotemporal spikes into unit-ascending order, while
extensions sliced its per-spike data (e.g. amplitudes, locations, etc.)
with a mask over the old (i.e. unpermuted) spike vector, causing a
discrepancy in ordering where row i of the extension data didn't match
row i of the new (permuted) spike vector.
There were warnings about this already in the code:
(although to be clear, the cause of the problem here wasn't on the
extension side, but on the Sorting side).
test_selection_preserves_parent_orderandtest_select_units_reordered_keeps_extension_alignment(sorry for thisname, open to suggestions) should hopefully prevent this from happening
again.
Commit 2
UnitSelectionSortingshares the parent's spike vector cache(s) onidentity selections (including rename).
Can save many GBs of memory. This also keeps a lazy Zarr spike vector
lazy (#4623 ).