Correct handling of time in plot_traces #3393
Conversation
src/spikeinterface/widgets/traces.py
Outdated
| @@ -673,13 +680,17 @@ def _get_trace_list(recordings, channel_ids, time_range, segment_index, return_s | |||
| assert all( | |||
| rec.has_scaleable_traces() for rec in recordings.values() | |||
| ), "Some recording layers do not have scaled traces. Use `return_scaled=False`" | |||
| frame_range = np.array( | |||
There was a problem hiding this comment.
Optionally, although I'm super curious about the history of this. We could start moving some of these internal variables over to sample instead of frame. Just for consistency. The function says time_to_sample_index. I think most people get the frame-sample equivalence, but it might be good to try to converge a bit more on one term for consistency. Trickiest thing is that the public function is frame_slice instead of sample_slice
| assert 0 <= start_frame < parent_size | ||
| assert ( | ||
| 0 <= start_frame < parent_size | ||
| ), f"'start_frame' must be fewer than number of samples in parent: {parent_size}" |
There was a problem hiding this comment.
This is a bit confusing in the case where start_frame is less than 0. It should just be two different checks:
assert 0 <= start_frame, "Requested `start_frame` is negative (requested timestamp may be earlier than the first timestamp in the recording)"
assert start_frame < parent_size, f"`start_frame` must be fewer than number of samples in parent: {parent_size}"
There was a problem hiding this comment.
I thought about saying this too, but I vaguely remember we discussed (and documented) not allowing negative slicing so I wasn't sure if this point was moot or not. It can't hurt to be more explicit though.
There was a problem hiding this comment.
I get this error trying to run recording_car.time_slice(0,1) when the recording starts around t0=15 seconds, I'm just assuming it's giving a negative start_frame. If it's somehow failing with start_frame > parent _size, that seems like a different issue
There was a problem hiding this comment.
No I agree that they are different problems. Although your error should likely be handled in the time_slice function since saying you have negative frames/samples isn't super helpful. @h-mayorquin wrote the time_slice right? so maybe that should be dealt with at the time level instead? What do you think Alessio and Heberto? If you give a bad time you should just know it right away in my opinion rather than getting a nonsensical frame?
There was a problem hiding this comment.
Dividing the asssertion in two each one with a message revealing what is the problem is good. Having specific assertions with proper messages in the time_slice is better. I also think all of that can be done in another PR as it is not really central to this.
|
It would be super nice to have tests for this as it's a little confusing as there are a lot of possible use cases to keep in mind. I think there was some discussion to avoid adding tests for widgets until a big widget refactor that was coming. Maybe we can make an issue to track these tests-to-add, otherwise adding tests now would be useful as they will catch regressions introduced in the impending refactor! |
| @@ -125,8 +125,10 @@ def __init__( | |||
|
|
|||
| if not rec0.has_time_vector(segment_index=segment_index): | |||
| times = None | |||
| t_start = 0 | |||
| t_end = rec0.get_duration(segment_index=segment_index) | |||
| t_start = rec0.sample_index_to_time(0, segment_index=segment_index) | |||
There was a problem hiding this comment.
I think that this first conditional can be completely removed now, as get_times() will always return a time array that will incorporate whether either a time vector, t_start attribute, or no time information set on the recording.
If times is None, then the times will be generated on the fly in _get_trace_list but this uses essentially the same code that is now implemented centrally in get_times().
I think this is also the case here. If the second case in the conditional can also be used and the first case removed, I think that times=None option could be removed from _get_trace_list.
So, basically this would centralise some of the time-related computations, away from the widget and out to the recording. But, maybe it will be necessary to check the frame computations are handled the same as there is some clipping done to them in one of the conditionals I linked. I think they should work as before as the times are converted already to frames with the new methods here.
There was a problem hiding this comment.
This was done to avoid loading the time vector if not needed. In the _get_traces_list, only a local time vector is generated on the fly
There was a problem hiding this comment.
Nice that makes sense
Co-authored-by: Zach McKenzie <[email protected]>
Fixes #3324