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

Kilosort templates can be all NaNs #28

Closed
lukebun opened this issue Sep 26, 2023 · 8 comments
Closed

Kilosort templates can be all NaNs #28

lukebun opened this issue Sep 26, 2023 · 8 comments

Comments

@lukebun
Copy link

lukebun commented Sep 26, 2023

Hello Julie,

I discovered that Kilosort is capable of outputting all NaNs for a template. As a result, bombcell will run into an error in "bc_waveformShape" when it tries to index into "max_waveform_value" which is empty.

My working solution is to put a try-catch around where "bc_waveformShape" is called in "bc_runAllQualityMetrics" and continue past the unit with the all NaN template.

I think it would be helpful for bombcell to be able to handle all NaN templates.

Thank you,
Luke

@Julie-Fabre
Copy link
Owner

Julie-Fabre commented Sep 27, 2023

Hi Luke,

Thanks for your message!
I've never seen that before. Which version of kilosort are you using? It doesn't make much sense to me that unit could have spikes times and no template waveform. Just to be sure of what's going on:

  • Do you get the error in the bc_waveformShape function?
  • What is the unit in question's maximum channel (qMetric.maxChannels(thisUnit)) ?

If a unit has no template waveform, it should be discarded. A try / catch statement will correctly handle this case, but I prefer to handle the specific issue (ie not computing the quality metrics if there is no waveform) because try/catch statements can mask some bugs and make it harder to troubleshoot.
I can add a check to only select units with waveforms before running bombcell's main loop, and then I suggest you pull that code or implement a similar change rather than having a try / catch statement.

Thanks again! Cheers,
Julie

@lukebun
Copy link
Author

lukebun commented Sep 27, 2023

Hi Julie,

I am using Kilosort 2.5. This unit does inexplicably have spikes assigned to it. I am also befuddled. I also reran Kilosort but it made the same NaN template.

  • Yes, the error is in bc_waveformShape. Specifically, line 103 "if max_waveform_value(end) > 0." Because max_waveform_value is a 1x0 empty array, it can't index into it.
  • The maximum channel of the unit is 1.

Thank you,
Luke

@Julie-Fabre
Copy link
Owner

How odd! I will modify bombcell to handle this special case. If you can, it might be useful to raise a github issue for Kilosort 2.5. This doesn't sound like normal behavior.

@Julie-Fabre
Copy link
Owner

Hi Luke,
I've created a branch to deal with waveforms that contain NaNs. By default, any of these units will not be classified (since it does not make any sense for waveforms to be NaNs). Here is the branch: https://github.com/Julie-Fabre/bombcell/tree/waveform_nans
Let me know if you run into any issues.
Have you had any luck figuring out what's going on in kilosort to create waveforms that are all NaNs?

@lukebun
Copy link
Author

lukebun commented Oct 10, 2023

Hi Julie,

Thank you so much!

As for the Kilosort side, I found another user who get all NaN templates in the Kilosort Issues. Unfortunately, the source of their problems seems entirely different than mine. I had intended to ask Nick Steinmetz about possible causes in person, but I didn't have the time during the meeting. So I've just raised an issue on the Kilosort page.

@Julie-Fabre
Copy link
Owner

That sounds good! Let me know if you figure it out, I am curious as to what is going on.

This was referenced Jan 8, 2024
@Julie-Fabre
Copy link
Owner

Hi Luke,
Someone else has run into the same issues as you (github issue #60) and I have integrated changes in the main branch to deal with these abnormal waveforms. You should be able to change back to this branch and be able to get any recent updates and feature upgrades.
I'm still confused about why kilosort is outputing these waveforms, but at least bombcell now handles it by default!

@lukebun
Copy link
Author

lukebun commented Jan 9, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants