Add support for kilosort>=4.0.12 #3055
Conversation
|
cool. |
| @@ -153,6 +160,11 @@ def _run_from_folder(cls, sorter_output_folder, params, verbose): | |||
| import torch | |||
| import numpy as np | |||
|
|
|||
| if verbose: | |||
| @@ -194,11 +206,17 @@ def _run_from_folder(cls, sorter_output_folder, params, verbose): | |||
| data_dir = "" | |||
| results_dir = sorter_output_folder | |||
| filename, data_dir, results_dir, probe = set_files(settings, filename, probe, probe_name, data_dir, results_dir) | |||
| ops = initialize_ops(settings, probe, recording.get_dtype(), do_CAR, invert_sign, device) | |||
| if version.parse(cls.get_sorter_version()) >= version.parse("4.0.12"): | |||
@chrishalcrow that's on the Kilosort side. Without the logger now only progress bars are printed... |
I'll make a separate PR for this |
There was a problem hiding this comment.
Look good! An extra note to the comments, save_sorting has a new parameter save_preprocessed_copy in v4.0.12. I don't know if we need to / want to explicitly support this at this stage.
I had a look through how the signature of all imported KS4 functions change across versions and they have been generally stable (except for changes addressed already in the SpikeInterface code). However, further to my below comment I think I would advocate using explicitly keyword arguments for all function calls. Although unlikely (but definitely not impossible over the timescale of years), some nasty bugs could be introduced if on the KS4 end arguments are: a) changed in order b) one removed and one added at the same time. We are lucky if they cause a crash but they could feasibly cause non-breaking changes to results 😱 .
| @@ -194,11 +206,17 @@ def _run_from_folder(cls, sorter_output_folder, params, verbose): | |||
| data_dir = "" | |||
| results_dir = sorter_output_folder | |||
| filename, data_dir, results_dir, probe = set_files(settings, filename, probe, probe_name, data_dir, results_dir) | |||
| ops = initialize_ops(settings, probe, recording.get_dtype(), do_CAR, invert_sign, device) | |||
| if version.parse(cls.get_sorter_version()) >= version.parse("4.0.12"): | |||
There was a problem hiding this comment.
I think this change was made in 4.0.11, could the condition be set to this version? kilosort code at 4.0.11
There was a problem hiding this comment.
actually, the change was made in 4.0.11 to get_run_parameters but in 4.012 to initialize_ops.
| @@ -194,11 +206,17 @@ def _run_from_folder(cls, sorter_output_folder, params, verbose): | |||
| data_dir = "" | |||
| results_dir = sorter_output_folder | |||
| filename, data_dir, results_dir, probe = set_files(settings, filename, probe, probe_name, data_dir, results_dir) | |||
| ops = initialize_ops(settings, probe, recording.get_dtype(), do_CAR, invert_sign, device) | |||
| if version.parse(cls.get_sorter_version()) >= version.parse("4.0.12"): | |||
| ops = initialize_ops(settings, probe, recording.get_dtype(), do_CAR, invert_sign, device, False) | |||
There was a problem hiding this comment.
could save_preprocessed_copy=False be used for readability?
In general I wonder if it would be better to call the kilosort functions with the kwargs explicitly e.g. initialize_ops(settings=settings, ...). This is annoyingly verbose but will be robust against changes to these function signatures (e.g. if they remove one argument and add another at the same time).
There was a problem hiding this comment.
Can you make a separate PR for this?
Plus some safe casting in the InjectTemplateRecording
Fixes #3050 #3020 #3007