Skip to content
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

Kilosort4 Wrapper #2529

Merged
merged 7 commits into from
Mar 5, 2024
Merged

Kilosort4 Wrapper #2529

merged 7 commits into from
Mar 5, 2024

Conversation

alejoe91
Copy link
Member

@alejoe91 alejoe91 commented Mar 1, 2024

Fixes #2527

Kilosort now has a SpikeInterface Wrapper (RecordingExtractorAsArray), so we don't need to save to bin!

I had to port subsections of the run_kilosort function to allow to:

  • do/skip drift correction
  • do/skip preprocessing

Added tests for all cases.

The parameters and descriptions were ported from their parameters definition.

One last comment:
The RecordingExtractorAsArray concatenates multiple segment. I think we should handle this externally to "unconcatenate" the sorting output, so I set handle_multi_segment to False.

@alejoe91 alejoe91 added the sorters Related to sorters module label Mar 1, 2024
@alejoe91 alejoe91 requested a review from samuelgarcia March 1, 2024 11:50
@alejoe91 alejoe91 mentioned this pull request Mar 1, 2024
2 tasks
@zm711
Copy link
Collaborator

zm711 commented Mar 1, 2024

One other point about the save to binary that might be interesting is that Mountaintsort5 had a speed up by switching to using the cached binary (see this). Kilosort also recommends using a binary file as well (but more so that you can use their gui and phy which are not really our concerns per se). I'm wondering if it might make sense to give the user an option to do this with write to binary to make this completely compatible with a Kilosort4 workflow for people or not. I could see arguments either way (and SI already has the export_to_phy so it doesn't really matter. Just curious what you think about this Alessio.

@alejoe91
Copy link
Member Author

alejoe91 commented Mar 1, 2024

Thanks Zach, that's a good point especially regarding the phy output. In my view, we should be as agnostic as possible to the sorter and just use the spike times outputs, so we can better compare and unify pipelines regardless of the sorter. In this spirit, I would rather not add this option and "force" the user to postprocess and export in SI. What do you think?

@zm711
Copy link
Collaborator

zm711 commented Mar 1, 2024

I think that's completely fair for the library. :) I guess if there are a lot of issues requesting something like that then there would be an impetus to do it and it could be reconsidered, but I agree this is the better initial choice. I think keeping the library agnostic is a good guiding principle, so thanks for framing it that way. Makes a lot of sense.

(And I already do all my postprocessing here anyway :) ).

@mhhennig
Copy link
Member

mhhennig commented Mar 2, 2024

Looks like the docker image is private right now - but with a pip installed KS4 this works as intended.

@alejoe91
Copy link
Member Author

alejoe91 commented Mar 2, 2024

Looks like the docker image is private right now - but with a pip installed KS4 this works as intended.

It's not pushed to docker hub yet, so it would need to be built locally ;) I'll test it a bit more and publish it

@samuelgarcia
Copy link
Member

Well done camarade.
Did they keep the same ouput format ?

@alejoe91
Copy link
Member Author

alejoe91 commented Mar 4, 2024

Well done camarade. Did they keep the same ouput format ?

Yes, same old ;)

@samuelgarcia
Copy link
Member

Could you also add it in documentation and the installation sorter page ?

@alejoe91
Copy link
Member Author

alejoe91 commented Mar 4, 2024

Right! Will do that on Friday @samuelgarcia

@alejoe91
Copy link
Member Author

alejoe91 commented Mar 4, 2024

@samuelgarcia ready to merge on my side. Added installation docs

Comment on lines +218 to +220
np.random.seed(1)
torch.cuda.manual_seed_all(1)
torch.random.manual_seed(1)
Copy link
Member

Choose a reason for hiding this comment

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

Not a great way!

@samuelgarcia
Copy link
Member

I merge this now in the main.
We will improve this in futuir PR.
So the bug fix 0.100.1 release will be able to have this fisrt KS4.

@samuelgarcia samuelgarcia merged commit e4ff263 into SpikeInterface:main Mar 5, 2024
11 checks passed
@alejoe91 alejoe91 deleted the kilosort4 branch April 12, 2024 08:59
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.

Implement Kilosort4 wrapper
4 participants