Skip to content
This repository was archived by the owner on Dec 10, 2021. It is now read-only.

Conversation

@alexmorley
Copy link
Collaborator

@alexmorley alexmorley commented Apr 30, 2019

Note this may also require the following changes to kilosort2 itself which can be found in my fork here: alexmorley/Kilosort2@43cbbff

Todo (here)

  • Expose more of the kilosort2 parameters to spikeforest
  • Allow no clusters to be found and (if there were no other errors) write the null result to a file
  • Write mda files in blocks to avoid reading entire recordings into memory [matlab]
  • Write mda files in blocks to avoid reading entire recordings into memory [sfmdarecordingextractors]
  • Don't copy the mda file from recording day to kilosort_dir (we are just going to convert it anyway)
  • Write the mda file as and 64bit integer to avoid loss of precision (currently it is a 32bit float, I have changed to 32bit integer for now just because the matlab mda implementation doesn't have a int64 option) This one is really important (hotfix in Kilosort(2) firings.mda dtype increase #47)
  • Re-base and test with new master

To do (kilosort2 fork)

  • Fix various assumptions that channel count will be high (> 32)
  • Fix (or warn) double detection of same spike by different templates

@magland
Copy link
Collaborator

magland commented Jun 8, 2019

@alexmorley , just getting to this now. What's the state of this, should we still try to merge?

@alexmorley alexmorley changed the title Fix Ksort Update Ksort'ers Jun 8, 2019
@alexmorley
Copy link
Collaborator Author

I've added a todo list for what I'd like to do before merging. I think we should because there are a couple of things that I think are quite important.

Most of the things on the list are implemented I just haven't pushed the changes yet.

@samuelgarcia
Copy link
Contributor

Alex,
@alexmorley : if you change some parameters here could you also change parameters in spiketoolkit.
https://github.com/SpikeInterface/spiketoolkit

Normally @magland should rebase spikeforest wrapper one by one on top on spiketoolkit wrappers.
The idea is to have a unique place for mofying parameters at wrappers level.

So if chnages occurs in between this rebase you could have differents parameters at the end.

best

Samuel

@alexmorley
Copy link
Collaborator Author

Yeah I'll make sure everything matches before we merge this no worries. Submitting my thesis in a couple weeks so just waiting till that's out of the way.

@samuelgarcia
Copy link
Contributor

Good luck to you.

@alejoe91
Copy link
Contributor

@alexmorley update on spikeforest/spiketoolkit sorter wrappers.

From the next release, they will both depend on spikesorters, so all the changes should be done there!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants