-
Notifications
You must be signed in to change notification settings - Fork 236
Add Kilosort output to SortingAnalyzer helper function #4202
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @chrishalcrow this is awesome! This has made many of my dreams come true. For sure this must be restricted to KS4 as you have done, as it simply not possible on older versions 🥲. Please see some suggestions but feel free to ignore any not helpful. I'm not so familiar with the internal workings of the sorting_analyzer so can' comment much on that aspect. *
On the testing front, I wonder if something like 1) have some kilosort output 2) read in the locations, amplitudes from .npy file on the test side and compare them to that in the loaded sorting analyzer. Similar for templates.npy, maybe a few templates can be cross-checked. It's not ideal because it is essentially re-computing everything in the test environment, but at least it will protect against regressions. Maybe this could be added to the kilosort4 test suite? I guess a danger is that the ks4 output format change in some way, and this would catch that. Otherwise it might be necessary to manually generate some mock KS4 data which might be a pain.
*[EDIT]
I think my only worry on that was if adding the mock recording object stores some metadata on the analyzer under the hood that is not erased when ._recording is set to None. I took a quick look at the code and it doesn't seem like it does now. But maybe in future this could cause a problem, but I think there is no workaround 🤔 maybe an assert somewhere they key metadata fields are empty?
| def kilosort_output_to_analyzer(folder_path, compute_extras=False, unwhiten=True) -> SortingAnalyzer: | ||
| """ | ||
| Load kilosort output into a SortingAnalyzer. | ||
| Output from kilosort version 4.1 and above are supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to check the version from the output? If not we should ask them to add on the KS repo as this would be a useful general addition. But maybe we could check that the kilosortX.log is not < 4? (IIRC that the logs are formatted in this way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have asked on KiloSort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you've run directly from kilosort, you'll have the version in kilosort4.log. So we could check there, and if it's not there, we have a guess...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that sounds good, if this tool is only for kilosort4 then just checking for the existing of that log file should do (unless it's extended to other versions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, the log files only appeared at v4.0.33. Thinking of other ways to check...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know of any files which KS2/2.5 defo don't have in their output, that KS4 does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I've added a _guess_kilosort_version function, to isolate this logic. Let's compare some outputs and see if we can make it reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like everything works fine for all versions (i.e. the probes, templates and spike locations are saved in the same way for all versions). So in the end we don't need to change behavior depending on version. We would have to if we got pcas working, but that's not gonna happen in this PR.
Co-authored-by: Joe Ziminski <55797454+JoeZiminski@users.noreply.github.com>
It does save some metadata related to the probe, and some basic info like sampling frequency in |
|
Hey @JoeZiminski . Just realised that the amplitudes saved by Kilosort are the "Per-spike amplitudes, computed as the L2 norm of the PC features for each spike." https://kilosort.readthedocs.io/en/latest/export_files.html So I don't think it's fair to call them |
Agree! |
| kilosort_log_file = Path(kilosort_path / "kilosort4.log") | ||
|
|
||
| if kilosort_log_file.is_file(): | ||
| return (4, 0, 33) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you want to parse the file here no ?
Also we check that the kilosort folder was lauch by spikeinterface and use the spikeinterface log to gete the version no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the idea is that we use whatever we can to figure out the kilosort version.
However, it seems like everything works fine for all versions (i.e. the probes, templates and spike locations are saved in the same way for all versions). So in the end we don't need to change behavior depending on version.
|
I'm happy with this now! In the end, after testing a load of output from Joe and Sai, we don't need the version checker. And we skip spike locations if we need to. |
This PR adds a converter that takes kilosort output and makes an analyzer from it. Ideal for using the gui on your sorter output.
Unsure how to test this - any good ideas?
To do:
compute_extrasImplement Kilosort version guesser