Support arbitrary multi probe and multi trigger structures in spikeglx #1150
Conversation
|
|
||
| data_interfaces = dict() | ||
| for stream in streams: |
There was a problem hiding this comment.
This is the critical part of this. Previously, the Converter assumed a fix structure of the files. This PR gets rids of that assumption.
| es_key: Optional[str] = None, | ||
| verbose: bool = True, | ||
| file_path: Optional[FilePath] = None, |
There was a problem hiding this comment.
I would like to deprecate this but I don't want this discussion to get in the way of this PR. If people who review this agree we could start a soft deprecation as we usually do.
Reminder: Spiekinterface takes folder path as input (as in this PR) and not file_path as we currently do in main.
| @@ -193,7 +194,7 @@ def test_electrode_table_writing(tmp_path): | |||
| np.testing.assert_array_equal(saved_channel_names, expected_channel_names_nidq) | |||
|
|
|||
| # Test AP | |||
| electrical_series = nwbfile.acquisition["ElectricalSeriesAPImec0"] | |||
| electrical_series = nwbfile.acquisition["ElectricalSeriesAP"] | |||
There was a problem hiding this comment.
Previously, the Converter always writes the ElectricalSeries using the name f"ElectricalSeriesAPImec{probe_index}" whereas the interface uses the name ElectricalSeriesAP only. This PR makes the behavior of the converter and the interface consistent in the single probe case hence the change.
| "ElectricalSeriesAPImec0": { | ||
| "name": "ElectricalSeriesAPImec0", | ||
| "description": "Acquisition traces for the ElectricalSeriesAPImec0." | ||
| "ElectricalSeriesAP": { |
There was a problem hiding this comment.
See this comment for the change:
https://github.com/catalystneuro/neuroconv/pull/1150/files#r1871529343
h-mayorquin
changed the title
|
The failing test is unrelated to this and should be fixed by #1151 |
| file_path: FilePath, | ||
| verbose: bool = True, | ||
| folder_path: Optional[DirectoryPath] = None, | ||
| stream_id: Optional[str] = None, | ||
| es_key: Optional[str] = None, | ||
| verbose: bool = True, | ||
| file_path: Optional[FilePath] = None, |
There was a problem hiding this comment.
Let's preserve the order of the args so we do not cause a breaking change for ordered args
| folder_path: Optional[DirectoryPath] = None, | ||
| file_path: Optional[FilePath] = None, |
There was a problem hiding this comment.
same here, let's keep file_path as the first arg for now
| self.folder_path = Path(file_path).parent | ||
|
|
||
| else: | ||
| self.stream_id = stream_id |
There was a problem hiding this comment.
could we check here to ensure that stream_id is one of the valid streams and give an error message with a list of valid streams if it is not?
There was a problem hiding this comment.
SpikeInterface already throws an error that is similar, example:
Does this work or would you like something more specific?
There was a problem hiding this comment.
ok great! Yeah that works
There was a problem hiding this comment.
Anything else missing to merge thigs?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1150 +/- ##
==========================================
+ Coverage 90.69% 90.87% +0.18%
==========================================
Files 129 129
Lines 8189 8209 +20
==========================================
+ Hits 7427 7460 +33
+ Misses 762 749 -13
Flags with carried forward coverage won't be shown. Click here to find out more.
|
SpikeGLXRecordingInterface interface to SpikeGLXRecordingExtractor extractor
#978
This should fix #1149 and #978