Skip to content

Conversation

@tayheau
Copy link
Contributor

@tayheau tayheau commented Dec 23, 2025

Following this todo

@tayheau tayheau marked this pull request as draft December 23, 2025 12:11
@tayheau tayheau marked this pull request as ready for review December 23, 2025 13:24
@alejoe91 alejoe91 added core Changes to core module testing Related to test routines labels Dec 23, 2025
Comment on lines +116 to +119
def _sorted_spike_vector(SX):
spikes = SX.to_spike_vector()
order = np.lexsort((spikes["sample_index"], spikes["unit_index"], spikes["segment_index"]))
return spikes[order]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this, since to_spike_vector should already return spikes sorted by segment_index + sample_index. Or did you add this to add the sorting by unit_index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pure inattention from me ! I remove it.

Copy link
Member

Choose a reason for hiding this comment

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

oups. this was needed put it back!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vacations will do me good XD

@alejoe91 alejoe91 added this to the 0.104.0 milestone Dec 23, 2025
@alejoe91
Copy link
Member

@tayheau I think we actually needed the unit_index lex sort after all! The reason is that before we were testing spiketrain-by-spiketrain, so the order didn't matter if we had synchronous spikes. Now with the spike vector representation we need to ensure unit_index is sorted too! Sorry about that! :)

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

Labels

core Changes to core module testing Related to test routines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants