Skip to content

Add archived sorters and change rt-sort to rtsort #4077

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 30, 2025

Conversation

alejoe91
Copy link
Member

No description provided.

@alejoe91 alejoe91 added the sorters Related to sorters module label Jul 21, 2025
@zm711
Copy link
Member

zm711 commented Jul 21, 2025

I think this is a great idea! Help makes it clear what we actively still support and what we don't!

@alejoe91 alejoe91 marked this pull request as ready for review July 28, 2025 08:14
# archived
# klusta="klusta",
# yass="yass",
# tridesclous="tridesclous"
Copy link
Member

Choose a reason for hiding this comment

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

Are we not doing tridesclous here anymore? I saw you had a commit un-archiving it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@samuelgarcia made a new release :)

@alejoe91 alejoe91 added this to the 0.103.0 milestone Jul 28, 2025
@zm711
Copy link
Member

zm711 commented Jul 28, 2025

@alejoe91
if you're changing the name for the wrapper do we want to change the reference? (for rtsort)?

https://spikeinterface.readthedocs.io/en/latest/references.html

I guess the reference could be hyphenated if that is how they refer to their own sorter? So our wrapper is unhyphenated and theirs is?

Copy link
Member

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

One note for us to remember in the future.

@@ -43,14 +43,14 @@
combinato="combinato",
herdingspikes="herdingspikes",
kilosort4="kilosort4",
klusta="klusta",
mountainsort4="mountainsort4",
mountainsort5="mountainsort5",
pykilosort="pykilosort",
Copy link
Member

Choose a reason for hiding this comment

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

pykilosort is only for < 3.10. I guess we are not there yet, but we are very close to needing to archive this. We should remember this.
https://github.com/MouseLand/pykilosort/blob/master/pyks2.yml

Copy link
Member

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

image

I think we should change this to rtsort since that is what the wrapper will have, and then leave the citation alone since the actual name of the sorter is RT-Sort. OR we leave the sorter as RT-Sort since that is the name in the citation. But I think we need to change this one line of the refs now that I've read everything.

@alejoe91
Copy link
Member Author

alejoe91 commented Jul 29, 2025

image I think we should change this to `rtsort` since that is what the wrapper will have, and then leave the citation alone since the actual name of the sorter is RT-Sort. OR we leave the sorter as RT-Sort since that is the name in the citation. But I think we need to change this one line of the refs now that I've read everything.

Good catch!!! done in last commti :)

Copy link
Member

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

Good by me now.

@alejoe91 alejoe91 merged commit ba6862f into SpikeInterface:main Jul 30, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sorters Related to sorters module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants