Reset-times: segment must have either time vector or sampling frequency

[alejoe91]

Fixes a bug in reset times that causes issues downstream, since rec segments must have either a sampling_frequency or a time_vector

[zm711]

I don't really touch the time vector stuff (I work in sample for everything). Should there be some additional testing with the time vector stuff to prevent this? Or are these some one off issues with the latest changes?

[alejoe91]

The problem arose in a preprocessing step. Basically a segment must always have either a time vector or a sampling frequency, otherwise instantiation fails

[JoeZiminski]

I think similar to #3324, if a self.t_start is set (but no time vector) this will not be reset? Should it be if self.has_time_vector(segment_index) or self.t_start is not None?

Out of interest, in what cases can a segment have a time vector but no sampling frequency set? is the sampling frequency not always propagated from parent recording to segment?

[alejoe91]

I think similar to #3324, if a self.t_start is set (but no time vector) this will not be reset? Should it be if self.has_time_vector(segment_index) or self.t_start is not None?

I think that if we set the time vector, then the segment.t_start should be forced to None, since they are mutually exclusive ways to set times.

Out of interest, in what cases can a segment have a time vector but no sampling frequency set? is the sampling frequency not always propagated from parent recording to segment?

The segment works in samples, so time information can be regarded as metadata. The sampling_frequency instead needs to be specified at the recording level.

[JoeZiminski]

Cool thanks I did not know that about sampling frequency!

I think if a time vector has been explicitly set by the user then that is the case. But it is possible that t_start only is set in some extractors, and then get_times() will generate the time_vector ad hoc but not save it to the recording. In this case you could have something like:

recording.get_times()  # 10, 10.001, ...., 11 (because recording.t_start = 10)
recording.reset_times()
recording.get_times() # # 10, 10.001, ...., 11

[alejoe91]

I see, got it! So then the reset_times should also have a segment.t_start = None?

[JoeZiminski]

I think the t_start on the segment is set to None in reset_times(), but the only problem is that this conditional will not be triggered in the case that t_start is set but not time_vector. And so even if the t_start is set, it will not be reset on reset_times(). I think it would have to be like:

for segment_index in range(self.get_num_segments()):

    rs = self._recording_segments[segment_index]
    if self.has_time_vector(segment_index) or rs.t_start is not None:
        rs.t_start = None
        rs.time_vector = None
        rs.sampling_frequency = self.sampling_frequency

Maybe there is a more elegant way to do this, I'm also not sure how the t_start case interacts with the rs.sampling_frequency case. But this will definitely make sure that times are reset even when only a t_start is on the segment but no time_vector.

I am wondering a bit more about the sampling_frequency on the segment itself, to me it's a bit confusing. My understanding is that it is enforced that all segments should have the same sampling frequency as the parent recording. A segment in will always have a baseline sampling frequency inherited from the recording regardless of whether a specific time vector has been set. And this sampling frequency will be the same as all the other segments (?). So for me it would be more intuitive for segments to be given a generic get_sampling_frequency() that just returns the parent recording sampling frequency, rather than each segment having its own optional attribute which implies the sampling frequencies can be different across segments, or somehow that a segment does not even have a sampling frequency. (sorry, this is somewhat outside the scope of this PR, if this makes any sense I can make an issue).

[alejoe91]

Got it! Then the reset_times() should always also set t_start=None, regardless of the segment having a time vector.

As for the sampling_frequency, note that the segment doesn't have any reference to the parent recording, so sampling_frequency must be passed externally.
Currently, we check that either a sampling_frequency or time_vector is passed, since these are the two alternative ways to convert samples/times and viceversa. The segment itself only works in the samples space to retrieve traces

[JoeZiminski]

@alejoe91 @samuelgarcia I think this current implementation will error out if self.has_time_vector is False right? because rs will not be defined

[alejoe91]

@alejoe91 @samuelgarcia I think this current implementation will error out if self.has_time_vector is False right? because rs will not be defined

ouch! I'll take a look!