Add spikeinterface support and and gin test for CEDRecordingInterface
#582
Conversation
|
This has the same error than the one in my personal system. It seems that at least temporary we will only be able to support the new format. |
Yup, that's fine. We've had trouble with |
Codecov Report
@@ Coverage Diff @@
## main catalystneuro/nwb-conversion-tools#582 +/- ##
==========================================
+ Coverage 88.34% 88.39% +0.04%
==========================================
Files 59 59
Lines 3192 3196 +4
==========================================
+ Hits 2820 2825 +5
+ Misses 372 371 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -37,6 +39,9 @@ def get_all_channels_info(cls, file_path: FilePathType): | |||
| assert HAVE_SONPY, INSTALL_MESSAGE | |||
| return cls.RX.get_all_channels_info(file_path=file_path) | |||
|
|
|||
| def __init__(self, file_path: FilePathType, smrx_channel_ids: list, verbose: bool = True): | |||
| def __init__(self, file_path: FilePathType, verbose: bool = True): | |||
There was a problem hiding this comment.
@h-mayorquin why did you remove this optional arg? iirc this was needed in a prior conversion
There was a problem hiding this comment.
Looks like due to
As discussed with @bendichter today, this did not have an associated gin test with spikeextractors so I am only adding spikeinterface support as it is.
SpikeInterface does not support this argument: https://github.com/SpikeInterface/spikeinterface/blob/master/spikeinterface/extractors/neoextractors/ced.py#L26
The prior conversion should be pinned to the older version of NCT+SpikeExtractors that supports it, so it should be fine.
Though one should probably go back and check if the new SpikeInterface version can resolve whatever the problem was with loading those channels - as I recall, the original SpikeExtractors version had to know which channels to read prior to __init__, but maybe they found a better workaround now (that was always a bit annoying to have to call a class method prior to instantiation). I'll see if I can dig those files up and send them to @h-mayorquin
There was a problem hiding this comment.
The old data share no longer exists (wasn't copied to the LBNL drive but instead was a link to their share, which has since been deleted) but I have a hard copy that I'm sending now.
There was a problem hiding this comment.
@h-mayorquin Confirmed that the previous .smrx files can be loaded in the new SpikeInterface without needing the extra argument (or class method)
There was a problem hiding this comment.
Can we please look into recreating this feature in the new pipeline?
There was a problem hiding this comment.
Selecting only the channels that you want to load, is that what you mean? (a form of channel stubbing?)
There was a problem hiding this comment.
getting the names of the channels as a class method and selecting only the channels you want to load as the init
There was a problem hiding this comment.
Ok, check:
Add spikeinterface support to
CEDRecordingInterface. Part of catalystneuro/neuroconv#60As discussed with @bendichter today, this did not have an associated gin test with
spikeextractorsso I am only addingspikeinterfacesupport as it is.I also had a problem running the old format (
.smr) tests in my own system but spikeinterface testing suite indicates that it should be supported. I am adding it here to see if it is a problem of local dependencies exclusive to me. If not, then probably we will have to support only the new format ("smrx") for a while until we figure this out.