Fix recording segment start time retrieval for spikeinterface>=0.104.1#1712
Fix recording segment start time retrieval for spikeinterface>=0.104.1#1712MGAMZ wants to merge 2 commits intocatalystneuro:mainfrom
spikeinterface>=0.104.1#1712Conversation
…interface>=0.104.1` and add legacy fallback for compatibility.
There was a problem hiding this comment.
Pull request overview
Refactors how NeuroConv retrieves recording segment start times to stay compatible with spikeinterface>=0.104.1 (where _recording_segments was removed) while keeping a fallback for older versions.
Changes:
- Replaced direct access to
recording._recording_segments[...].t_startwith a compatibility helper_get_recording_segment_start_time. - Added/updated tests to verify shifted recordings correctly write
starting_time, and added a legacy fallback test. - Removed old test hacks that directly mutated
recording._recording_segments.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/neuroconv/tools/spikeinterface/spikeinterface.py |
Adds _get_recording_segment_start_time and uses it when writing ElectricalSeries/TimeSeries starting times. |
tests/test_modalities/test_ecephys/test_tools_spikeinterface.py |
Adds tests for shifted start time and legacy fallback (currently introduces a test discovery issue). |
tests/test_modalities/test_ecephys/test_ecephys_interfaces.py |
Removes legacy _recording_segments mutation in stub test. |
| def test_get_recording_segment_start_time_legacy_fallback(): | ||
| recording = _LegacyRecording(t_start=1.5) | ||
|
|
||
| start_time = _get_recording_segment_start_time(recording=recording, segment_index=0) | ||
|
|
||
| assert start_time == 1.5 | ||
|
|
||
| def test_write_as_lfp(self): | ||
| write_as = "lfp" |
There was a problem hiding this comment.
test_get_recording_segment_start_time_legacy_fallback is defined at module scope, but the subsequent def test_write_as_lfp(self): appears indented under it. This nests the rest of the TestAddElectricalSeriesWriting test methods inside a function, so they won’t be discovered/executed by unittest/pytest. Move the legacy-fallback test elsewhere (e.g., after the class) and restore the class method indentation for the following tests.
|
|
||
| def _get_recording_segment_start_time(recording: BaseRecording, segment_index: int) -> float: | ||
| if hasattr(recording, "get_start_time"): | ||
| return float(recording.get_start_time(segment_index=segment_index)) |
There was a problem hiding this comment.
recording.get_start_time(...) can legitimately return None when the recording has no explicit start time. Calling float(None) will raise TypeError and reintroduce the original failure mode that ... or 0 previously avoided. Treat None as 0.0 before converting to float.
| return float(recording.get_start_time(segment_index=segment_index)) | |
| start_time = recording.get_start_time(segment_index=segment_index) | |
| return 0.0 if start_time is None else float(start_time) |
| segment = segments[segment_index] | ||
| if hasattr(segment, "get_start_time"): | ||
| return float(segment.get_start_time()) | ||
| if hasattr(segment, "t_start"): |
There was a problem hiding this comment.
segment.get_start_time() may also return None (depending on spikeinterface version/segment implementation). This path currently does float(segment.get_start_time()) and will raise TypeError for recordings without an explicit start time. Handle a None return by defaulting to 0.0 (consistent with the t_start/_t_start branches).
SpikeInterface/spikeinterface#4462 introduces breaking change,
BaseRecording._recording_segmentsno longer exists, and a new propertyBaseRecording.segmentsis available.This issue causes NeuroConv
_add_recording_segment_to_nwbfilefunction to fail.This PR introduces a compatibility function
_get_recording_segment_start_timewhich can handle bothspikeinterface<=0.104.0andspikeinterface>=0.104.1.The test unit is updated too.