Updates to kilosort 4: version >= 4.0.16, bad_channels, clear_cache, use_binary_file

[chrishalcrow]

As discussed in #3316, we decided to only support the latest version of kilosort. This PR checks the version and makes the arguments consistent with the kilosort arguments for version 4.0.16.

I added the version check by extending initialize_folder, which is where the installation check is. It's a little awkward because we should do the installation check before the version check, but this doesn't quite fit in the structure of the BaseSorter initialize_folder method. So we actually check that kilosort is installed twice.

[alejoe91]

@chrishalcrow @JoeZiminski

I would suggest using this PR also to:

• Update the Kilosort4 action
• Add the use_binary_file param and logic: CUDA out of memory error from running kilosort #3321 (comment)
• Make sure docker image is up-to-date: PR here Update KS4 to 4.0.16 spikeinterface-dockerfiles#81

[alejoe91]

@chrishalcrow I'll push a change for the use_binary_file directly to this PR (updated title)

[alejoe91]

Actually, if the recording is binary I would always do this path. It's faster.

@zm711 @samuelgarcia do you agree?

[zm711]

That makes sense to me. KS4 likes the binary so if that's an option I think feeding it that is better.

[alejoe91]

done! the only caveat is that if one has a binary recording, but wants to force the RecordingExtractorAsArray mode, the only way is to modify the recording, e.g.: recording_non_bin = spre.scale(recording_bin)

[alejoe91]

done and added a test that checks that results are the same: https://github.com/chrishalcrow/spikeinterface/blob/only-allow-above-ks-4-16/.github/scripts/test_kilosort4_ci.py#L401

[alejoe91]

@chrishalcrow @zm711 @JoeZiminski also updated the KS4 actions. Now only versions >= 4.0.16 are tested.

A test run on a tmp branch is running here: https://github.com/SpikeInterface/spikeinterface/actions/runs/10670770848/job/29575421194

Let me know what you think!

[alejoe91]

@JoeZiminski I added a trigger for changes to the kilosort4 file and a general cleanup of the ks4 tests.

Let me know what you think!

[JoeZiminski]

Hey @alejoe91 sorry I never got around this in good time, thanks for taking care of it and the clean up! Looks great, also learnt some cool SI functions I did not know about it. Just one comment above. Cheers!

[JoeZiminski]

Great! @alejoe91 before merging this does it make sense to release SI patch 0.101.1?

[alejoe91]

Great! @alejoe91 before merging this does it make sense to release SI patch 0.101.1?

I would actually include it in the release:) It's a great upgrade!

[JoeZiminski]

Oh yes that makes sense @alejoe91, I guess if there were no changes to the KS4 code that dealt with KS4 versions <=4.0.16 that have happened between 0.101.0 and this PR (I don't think there were?) then that is better! 0.101.0 is last version that supports KS4 <4.0.16 and >=0.101.1 supports KS4 4.0.16. sounds good!

[JoeZiminski]

Sorry for the back and forth @alejoe91, unless I'm mistaken a lot of the commits for finalising KS4 versions <4.0.16 were added after 0.101.0 was released (e.g. #3085, #3276, I think #3265). So it would be cleaner to make a patch version before merging this PR for a clean break. However, it's not the end of the world if not, but the 'transition point' would be the commit before this is merged rather than a full version which might be confusing.

[alejoe91]

Hey @JoeZiminski

#3265 is for KS<4. For the other changes, they are all to update the most recent KS4 version, including the CI changes. So I think it's ok to have the new release as a breaking point.

In addition, since we rely on many external tools, I think that in this case a patch release is also ok. We should encourage people to update KS4 anyways

[JoeZiminski]

I think #3276 adds save_preprocesed_copy (old spelling) which is for KS <4.0.13 (the typo is fixed in 4.0.13). That CI PR is to test <4.0.13 (?) also I think and then is overwritten here, but the CI stuff doesn't matter so much. I think if you want the 'final final version' of spikeinterface that supports KS4 <4.0.13 including tests you would need to go to the commit prior to this one as 0.101.0 will be missing save_preprocessed_copy (and tests, that matters less). I think this is an extreme edge case though and so happy to squash it into 0.101.1, in practice I think it's unlikely anyone will need such a specific version!

[chrishalcrow]

Looks good, and is working for me locally.

I don't think we need a release that supports in-between kilosort versions, since it was rapidly developing in its first months. I'd vote to have that if there had been a long-stable release, followed by a significant update.

EDIT: and I'd never been in the scripts folder before: cool!

[alejoe91]

@zm711 @JoeZiminski @chrishalcrow

Waiting for tests to pass and merging this :)

[guptadivyansh]

I would just like to report here that use_binary_file makes a big difference to KS4 sorting performance. Writing the binary does take a while but overall I managed to sort a ~2 hour recording in ~4 hours with use_binary_file = True compared to ~22 hours with the default use_binary_file = False for the same recording. So, thank you for adding this param!

I would even suggest making it the default behavior unless there is a good reason/policy against using disk space.

Workstation: Intel i7 8700K, Nvidia 3060 12GB, 64GB RAM, SSD

[zm711]

I argued for making that the default so I'm in favor! It might actually be worth writing this out in a new separate issue since this is a merged PR. That will gives us the opportunity to discuss it again and make it discoverable to other users so we can see if there is community desire for this. Finally it will allow other users to discover this feature so they might test it out :)

[samuelgarcia]

I was the actual default but given this enormous diff, I guess we should move.
It is hard to understand why this is so slow using the spikeinterface API.