-
Notifications
You must be signed in to change notification settings - Fork 186
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
Add kilosort drift maps (that work directly on kilosort output) #3499
base: main
Are you sure you want to change the base?
Add kilosort drift maps (that work directly on kilosort output) #3499
Conversation
@JoeZiminski we have a very similar plotting functio: Also can you remove the npy files from the PR? |
Hi @alejoe91 thanks for this, I removed those test files! That makes sense, I was in two minds about this as the input data formats and computations are quite closely linked to the kilosort drift map implementation and would bloat the module, and make the signature very long. I was thinking about discarding them but it would be nice to have these options so MATLAB users who are porting to SpikeInterface have the same functionality. However I agree there is loads of code duplication so its not ideal. How about one of these two possibilities: Have two functions Alternatively, all features from the |
I think I agree with Alessio. I would prefer to enhance the existing plotting function we already have. Also about having plotting function that are sorters specific, this is not in main idea of spikeinterface which is globally sorter neutral. What we could have ks specific utils function that tranform the ks folder into spikeinterface objects (analyser, motion, ...) and then make plotting function on top on spikeinterface objects. In short I would do:
with this in mind, the enhancement for |
Thanks @samuelgarcia, I agree this is a much nicer approach. Would you be happy for all features from the KS drift map (e.g. histogram, peak coloring, drift events) to be added to the It would be awesome to fully load the kilosort output directory to an analyzer object, however I think the work that would take is beyond what I could do at this time. Would you be happy for now with just loading the peaks? Even though it's not as nice overall, it would would be sufficient for the raster map e.g.:
|
This PR adds kilosort-style drift maps calculated directly from the kilosort output, ported from Nick Steinmetz's spikes repository (with permission from Nick, and indicated in relevant docstrings).
The benefit of this functionality is that it works directly on the kilosort output and provides a nice overview of all detected spikes and drift. This is very useful for assessing the outputs of various preprocessing steps in SpikeInterface and kilosort. At present, if you want to see the effects of skipping a kilosort preprocessing step it is hard to assess the results (the kilosort preprocessed
temp_wh.dat
is a bit tricky to read consistency across KS versions) and there is no ground truth for real data so comparing the sorting outputs directly is not so helpful. The plot is also very pretty!@alejoe91 @samuelgarcia this PR uses gridspec to define the size of the subplots (
gs = fig.add_gridspec(1, 2, width_ratios=[1, 5])
) and I couldn't find a way to do this withmake_mpl_figure
so I did not use it for this widget. I am sure not usingmake_mpl_figure
is a bad idea for many architectural reasons I don't know about it, but on of off chance it's okay, I ask: a) may I not usemake_mpl_figure
b) if it is required, is it okay to add an option to define the relative width of subplots?Example code to run:
Example plots
Original MATLAB version
Python version from this PR
TODO