Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent handling of time offsets in plot_traces #3324

Open
jonahpearl opened this issue Aug 21, 2024 · 6 comments · May be fixed by #3393
Open

Inconsistent handling of time offsets in plot_traces #3324

jonahpearl opened this issue Aug 21, 2024 · 6 comments · May be fixed by #3393
Labels
widgets Related to widgets module

Comments

@jonahpearl
Copy link
Contributor

Hi all — I was doing some manual digging around to deal with artifacts caused by some custom lighting in our rigs, and I wanted to find the moment in the recording where the artifacts began. I noticed that depending on whether I used recording.time_slice() or recording.plot_traces, there are two different behaviors for dealing with the case where t0 != 0 (this is coming from open ephys, so it's often the case that the first timestamp isn't 0 in the recording).

Case 1: the "give me X many seconds into the recording" strategy. This appears to be used by recording.plot_traces, and I think is a bug.

Case 2: the "give me the data where the timestamps equal this value" strategy. This appears to be used by recording.time_slice, and I think is the correct way (?).

For example, here is the plot from plot_traces for the time range (25.5, 26.5): you can see the artifacts start ~halfway through the window.
image

Now if I run this code to try to show the same thing but with recording.time_slice(), we instead get a different moment:

tmp_rec = recording_car.time_slice(25.5, 26.5)
t = tmp_rec.get_traces(channel_ids=['CH40'])
plt.figure(figsize=(10,3))
plt.plot(t, lw=0.5)

image

If we adjust the time slicing to account for the inconsistent behavior, we recover the same moment in the data:

t0 = recording_car.get_times()[0]
tmp_rec = recording_car.time_slice(25.5 + t0, 26.5 + t0)  # this will get data from where the timestamp equals 25.5 + t0
t = tmp_rec.get_traces(channel_ids=['CH40'])
plt.figure(figsize=(10,3))
plt.plot(t, lw=0.5)

image

You can see the behavior in the code. time_slice ultimately relies on this logic (at least in my case, it seems that self.t_start is assigned automatically when reading from OE folders): sample_index = (time_s - self.t_start) * self.sampling_frequency, which means that, say, if t0 were 10, and the user requested t=20, the code would correctly give the user the data from 10 seconds into the recording.

However the plot/get_traces functions don't appear to do this correction, as seen here and down on L145 below that, and then the helper function just inherits that time range directly.

Hopefully fixing the behavior in plot_traces is easy enough and won't hurt anyone's workflow — I imagine that may be why the bug still exists :)

@alejoe91 alejoe91 added the widgets Related to widgets module label Aug 21, 2024
@alejoe91 alejoe91 added this to the 0.101.1 milestone Aug 27, 2024
@JoeZiminski
Copy link
Collaborator

JoeZiminski commented Sep 9, 2024

Hey @jonahpearl thanks for this, so to be sure I understand correctly:

  1. plot_traces() is getting the times 25.5 - 25.6 into the recording, irregardless of what the actual start time is.
  2. the expected behaviour is that plot_traces() displays the actual 25.5 - 25.6 second range. So for example, if the recording was from ground-truth times 20 to 50 seconds, you would expect the time slice you used (25.5, 25.6) to display 5.5 to 5.6 seconds into the recording (25.5-25.6 true time). But, it is giving 25.5 to 25.6 seconds into the recording (which is 45.5 to 45.6 seconds true time).

I think this is because the plot_traces() code you linked is looking for a time_vector but ignoring any t_start set on the recording. Presumably if you do recording_car.has_time_vector() you will get False? Could you do recording_cmr.set_times(recording_cmr.get_times()) and check if that fixes the problem? (otherwise I've misunderstood something).

Now, this conditional is outdated as get_times() will generate times on the fly even if no time vector is found. This is better as it will use any t_start on the recording. I think conditional block in plot_traces linked above can now be replaced with just the second part of the conditional:

times = rec0.get_times(segment_index=segment_index)
t_start = times[0]
t_end = times[-1]

@alejoe91 @samuelgarcia do you agree? In practice, can we deprecate has_time_vector() as one is always generated now, and it will use t_start if available whereas legacy code that uses this conditional check may not?

@jonahpearl
Copy link
Contributor Author

Hi @JoeZiminski — yes, running recording_cmr.set_times(recording_cmr.get_times()) aligns the two behaviors, as you expected :) thanks!

One other note, if I run that fix, and then "naively" ask for a time segment outside the start of the recording with the plot_traces function, I get a fairly cryptic ValueError because it's trying to run operations on an empty array, it might be worth checking for this and raising a user-readable error, since I imagine some people will just instinctively try to plot the first few seconds of their recording. recording.time_slice() gives an AssertionError which is clear from context if you look at the traceback, but it wouldn't be hard to add a simple message giving the user feedback.

ValueError Traceback
ValueError                                Traceback (most recent call last)
Cell In[19], line 4
      1 fig, ax = plt.subplots(figsize=(15,20))
      3 iids = slice(39,40)
----> 4 si.plot_traces(
      5     recording_car, 
      6     backend='matplotlib',  
      7     order_channel_by_depth=True,
      8     # time_range=(25.5, 26.5),  # started by 34
      9     time_range=(1,2),
     10     channel_ids=recording_car.channel_ids[iids],
     11     mode="line",
     12     ax=ax,
     13     show_channel_ids=True,
     14 )
     15 _ = plt.yticks(np.arange(iids.stop - iids.start), recording_car.channel_ids[iids])

File ~/datta-lab/spikeinterface/src/spikeinterface/widgets/traces.py:170, in TracesWidget.__init__(self, recording, segment_index, channel_ids, order_channel_by_depth, time_range, mode, return_scaled, cmap, show_channel_ids, events, events_color, events_alpha, color_groups, color, clim, tile_size, seconds_per_row, scale, with_colorbar, add_legend, backend, **backend_kwargs)
    168 traces0 = list_traces[0]
    169 mean_channel_std = np.mean(np.std(traces0, axis=0))
--> 170 max_channel_amp = np.max(np.max(np.abs(traces0), axis=0))
    171 vspacing = max_channel_amp * 1.5
    173 if rec0.get_channel_groups() is None:

File ~/miniconda3/envs/spikeinterface/lib/python3.9/site-packages/numpy/core/fromnumeric.py:2810, in max(a, axis, out, keepdims, initial, where)
   2692 @array_function_dispatch(_max_dispatcher)
   2693 @set_module('numpy')
   2694 def max(a, axis=None, out=None, keepdims=np._NoValue, initial=np._NoValue,
   2695          where=np._NoValue):
   2696     """
   2697     Return the maximum of an array or maximum along an axis.
   2698 
   (...)
   2808     5
   2809     """
-> 2810     return _wrapreduction(a, np.maximum, 'max', axis, None, out,
   2811                           keepdims=keepdims, initial=initial, where=where)

File ~/miniconda3/envs/spikeinterface/lib/python3.9/site-packages/numpy/core/fromnumeric.py:88, in _wrapreduction(obj, ufunc, method, axis, dtype, out, **kwargs)
     85         else:
     86             return reduction(axis=axis, out=out, **passkwargs)
---> 88 return ufunc.reduce(obj, axis, dtype, out, **passkwargs)

ValueError: zero-size array to reduction operation maximum which has no identity
AssertionError Traceback
AssertionError                            Traceback (most recent call last)
Cell In[20], line 1
----> 1 tmp_rec = recording_car.time_slice(1, 2)

File ~/datta-lab/spikeinterface/src/spikeinterface/core/baserecording.py:727, in BaseRecording.time_slice(self, start_time, end_time)
    724 start_frame = self.time_to_sample_index(start_time) if start_time else None
    725 end_frame = self.time_to_sample_index(end_time) if end_time else None
--> 727 return self.frame_slice(start_frame=start_frame, end_frame=end_frame)

File ~/datta-lab/spikeinterface/src/spikeinterface/core/baserecording.py:702, in BaseRecording.frame_slice(self, start_frame, end_frame)
    684 """
    685 Returns a new recording with sliced frames. Note that this operation is not in place.
    686 
   (...)
    697     A new recording object with only samples between start_frame and end_frame
    698 """
    700 from .frameslicerecording import FrameSliceRecording
--> 702 sub_recording = FrameSliceRecording(self, start_frame=start_frame, end_frame=end_frame)
    703 return sub_recording

File ~/datta-lab/spikeinterface/src/spikeinterface/core/frameslicerecording.py:37, in FrameSliceRecording.__init__(self, parent_recording, start_frame, end_frame)
     35     start_frame = 0
     36 else:
---> 37     assert 0 <= start_frame < parent_size
     39 if end_frame is None:
     40     end_frame = parent_size

AssertionError:

@alejoe91
Copy link
Member

I think this should be fixed if we use time_slice in plot_traces, since it seems to handle correctly all cases. Will make a PR

@alejoe91 alejoe91 linked a pull request Sep 11, 2024 that will close this issue
@alejoe91
Copy link
Member

@jonahpearl could you test this? #3393

@alejoe91
Copy link
Member

Also improved error messages in that PR

@jonahpearl
Copy link
Contributor Author

@jonahpearl could you test this? #3393

Actually doesn't seem to work for me — when I subselect my one channel, I just get an empty plot, even without passing any time_range. If I don't subselect the channels it works. And you can see that channel has data b/c if i pull it out directly it's fine.

image image image

@alejoe91 alejoe91 removed this from the 0.101.1 milestone Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
widgets Related to widgets module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants