Fix gaussian filter with time vector #4268
Conversation
alejoe91
added
core
|
@alejoe91 Tested this and can confirm: fixes the issue. Thanks! |
| sigmas = [] | ||
| if freq_min is not None: | ||
| sigmas.append(sf / (2 * np.pi * freq_min)) | ||
| sigmas.append(self.sampling_frequency / (2 * np.pi * freq_min)) |
There was a problem hiding this comment.
In case of using a time_vector, could self.sampling_frequency be None?
There was a problem hiding this comment.
As I understand it:
recording.sampling_frequency will not be None, even after recording.set_times() is called. This is because recording.set_times() will set the segment's sampling_frequency = None, but does nothing to recording._sampling_frequency.
In other words:
recording.set_times(t)
assert recording.sampling_frequency is not None
assert recording._recording_segements[0].sampling_frequency is NoneI think this is the intended behavior.
Then, the recording-level gaussian filter object (GaussianFilterRecording) creates segment-level gaussian filter objects (GaussianFilterRecordingSegment), supplying each with recording.sampling_frequency. Upon instantiation, these segment-level objects then set their own self.sampling_frequency = my_parent_recording_sampling_frequency, which for the reasons above will be non-None.
So in the line you highlighted here, self.sampling_frequency will not be None, even though a segment's self.sampling_frequency should and would be None "normally" when time_vector is present.
@alejoe91 What I am not sure is intended behavior, is that this un-does the nullifying of the segment sampling frequencies that was performed by recording.set_times().
In other words:
recording.set_times(t)
assert recording.sampling_frequency is not None
assert recording._recording_segements[0].sampling_frequency is None
recording = sp.gaussian_filter(recording, ...)
assert recording.sampling_frequency is not None
assert recording._recording_segements[0].sampling_frequency is None # THIS FAILS NOW!There was a problem hiding this comment.
Thanks @grahamfindlay , very helpful summary.
So GaussianFilterRecordingSegment will have a not None sampling_frequency, even when the input RecordingSegment's sampling frequency is None. Which might be fine, but might be unexpected.
There was a problem hiding this comment.
This was actually an issue with the Zarr reader, where if time vector was present, sampling frequency was not set. Now this is fixed, but the GaussianFilter still uses the parent recording sampling freq, which is safer
4 participants
Fixes #4267
@grahamfindlay could you check this out? I think it fixes your issue and makes the behavior less ambiguous