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 :)