-
Notifications
You must be signed in to change notification settings - Fork 189
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
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.:
|
Thanks all, I will close this in favour of two separate PRs (this PR can act as a record of a pure port of these |
314a598
to
606ed1c
Compare
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. 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