Skip to content

Check unit order in UnitsSelectionSorting against the parent's ids - #4814

Open
h-mayorquin wants to merge 1 commit into
SpikeInterface:mainfrom
h-mayorquin:order_check_on_parent_rename
Open

h-mayorquin wants to merge 1 commit into
SpikeInterface:mainfrom
h-mayorquin:order_check_on_parent_rename

Conversation

@h-mayorquin

Copy link
Copy Markdown
Contributor

This is in the context of optimizations related to #4811. This s a single line change.

When looking at the memory used by rename_units I realized that the check added in #4581 to skip the np.lexsort when the order of the units is preserved never passes for a rename. It searches the renamed ids (self.unit_ids) in the parent ids, and as they are not there np.searchsorted returns the same position for every unit (array([2, 2]) for a rename of two units):

# check if order is preserved
pos = np.searchsorted(self._parent_sorting.unit_ids, self.unit_ids)
order_is_preserved = np.all(np.diff(pos) > 0)

Plus, np.searchsorted assumes that the parent ids are sorted. A selection that keeps the order of a parent with unsorted ids, string ids for example, also pays for the sort. This PR gets the positions with ids_to_indices(self._unit_ids) instead, and both cases now skip the sort.

@h-mayorquin h-mayorquin self-assigned this Oct 2, 2026
@h-mayorquin h-mayorquin added core Changes to core module performance Performance issues/improvements labels Oct 2, 2026
@alejoe91 alejoe91 added this to the 0.105.1 milestone Oct 5, 2026
alejoe91
alejoe91 previously approved these changes Oct 5, 2026

@alejoe91 alejoe91 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!

@alejoe91
alejoe91 requested a review from grahamfindlay October 6, 2026 08:45
@alejoe91

alejoe91 commented Oct 6, 2026

Copy link
Copy Markdown
Member

LGTM! @grahamfindlay do you want to take a look?

@alejoe91
alejoe91 dismissed their stale review October 6, 2026 10:22

other PR

@alejoe91

alejoe91 commented Oct 6, 2026

Copy link
Copy Markdown
Member

@h-mayorquin actually this other PR #4783 removes that logic alltoghether

@samuelgarcia

Copy link
Copy Markdown
Member

bravo

@h-mayorquin

Copy link
Copy Markdown
Contributor Author

Yes, I am OK with having the other PR instead of this if they achieve the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Changes to core module performance Performance issues/improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants