Remove some full lexsort of spikes when not necesary.#4618
Open
samuelgarcia wants to merge 13 commits into
Open
Remove some full lexsort of spikes when not necesary.#4618samuelgarcia wants to merge 13 commits into
samuelgarcia wants to merge 13 commits into
Conversation
…"sample_index"], spikes["segment_index"]))]
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Member
|
@samuelgarcia can you check father failing tests? Seems related to MDA sorting tests. Maybe we need to force lexsort there |
alejoe91
added
core
…remove_some_lexsort
for more information, see https://pre-commit.ci
…terface into remove_some_lexsort
alejoe91
reviewed
Jun 29, 2026
alejoe91
reviewed
Jun 29, 2026
Member
|
@samuelgarcia justa couple of minor comments |
alejoe91
reviewed
Jun 29, 2026
Co-authored-by: Alessio Buccino <alejoe9187@gmail.com>
alejoe91
reviewed
Jun 29, 2026
chrishalcrow
reviewed
Jul 3, 2026
| unit_indices = np.concatenate(unit_indices) | ||
|
|
||
| order = np.argsort(sample_indices) | ||
| order = np.argsort(sample_indices, stable=True) |
Member
There was a problem hiding this comment.
Just checking: the reason we do this is because an argsort with stable=True will better respect the original data (for a small number of edge cases)?
chrishalcrow
reviewed
Jul 3, 2026
Comment on lines
+896
to
+898
| # here we only do a sort by indices a lexsort on sample_indices | ||
| # the stable=True is equivalent to np.lexsort((unit_indices, sample_indices, )) | ||
| # because because we construct by looping on unit_ids |
Member
There was a problem hiding this comment.
Suggested change
| # here we only do a sort by indices a lexsort on sample_indices | |
| # the stable=True is equivalent to np.lexsort((unit_indices, sample_indices, )) | |
| # because because we construct by looping on unit_ids | |
| # here, a sort by indices with stable=True is equivalent to | |
| # np.lexsort((unit_indices, sample_indices)) | |
| # because we construct by looping on unit_ids |
chrishalcrow
reviewed
Jul 3, 2026
Comment on lines
+917
to
+918
| # the spikes are not lexsorted here because the previous loop ensure that the spike vector is constructucted alway the same way. | ||
| # spikes = spikes[np.lexsort((spikes["unit_index"], spikes["sample_index"], spikes["segment_index"]))] |
Member
There was a problem hiding this comment.
Suggested change
| # the spikes are not lexsorted here because the previous loop ensure that the spike vector is constructucted alway the same way. | |
| # spikes = spikes[np.lexsort((spikes["unit_index"], spikes["sample_index"], spikes["segment_index"]))] |
chrishalcrow
reviewed
Jul 3, 2026
Comment on lines
+191
to
+192
| # the spikes do not need a full lexsort because synthesize_poisson_spike_vector() garanty spikes to sorted by frame already | ||
| # spikes = spikes[np.lexsort((spikes["unit_index"], spikes["sample_index"], spikes["segment_index"]))] |
Member
There was a problem hiding this comment.
Suggested change
| # the spikes do not need a full lexsort because synthesize_poisson_spike_vector() garanty spikes to sorted by frame already | |
| # spikes = spikes[np.lexsort((spikes["unit_index"], spikes["sample_index"], spikes["segment_index"]))] |
chrishalcrow
reviewed
Jul 3, 2026
| # Sort globaly | ||
| spike_frames = spike_frames[:num_correct_frames] | ||
| sort_indices = np.argsort(spike_frames, kind="stable") # I profiled the different kinds, this is the fastest. | ||
| # the stable is important because this guarantees to be equivalent to |
Member
There was a problem hiding this comment.
Suggested change
| # the stable is important because this guarantees to be equivalent to | |
| # the `stable` is important because this guarantees result is equivalent to |
chrishalcrow
reviewed
Jul 3, 2026
| s2 = SX2.to_spike_vector() | ||
| if not check_exact_lexsort: | ||
| # 2 sorting can be equal even if the internal lexsort is not the same. | ||
| # spiketrains still wiwll be the same per units |
Member
There was a problem hiding this comment.
Suggested change
| # spiketrains still wiwll be the same per units | |
| # spiketrains still will be the same per units |
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
4 participants
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.
See discussion: #4606
We do not need strong fulll lexsort in some place because it is not a need.
Important notes : during the version 0.104.0 the zarr format was lexsorting on the fly in init.
This was useless because the lexsort was done before saving in the analyzer create mechanisim.
So we do not need any backward compatibility startegy to handle this.
This should be merge before #4579