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.

Sign up for GitHub

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

Correct handling of time in plot_traces #3393

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Correct handling of time in plot_traces #3393

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
53ee86a
Correct handling of time in plot_traces
alejoe91 Sep 11, 2024
e225fd5
Add more checks and error messages
alejoe91 Sep 11, 2024
c9a0e64
Apply suggestions from code review
alejoe91 Oct 24, 2024
01a21ce
Merge branch 'main' into fix-3324
alejoe91 Oct 24, 2024
0b6d49f
frame_range to sample_range
alejoe91 Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/spikeinterface/core/frameslicerecording.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ def __init__(self, parent_recording, start_frame=None, end_frame=None):
if start_frame is None:
start_frame = 0
else:
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}"
alejoe91 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Thanks Heberto.


if end_frame is None:
end_frame = parent_size
Expand Down
5 changes: 5 additions & 0 deletions src/spikeinterface/widgets/traces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,11 @@ def __init__(
)
time_range[1] = t_end

if time_range[0] < t_start or time_range[1] < t_start:
raise ValueError(f"All time_range values must be greater than {t_start}")
alejoe91 marked this conversation as resolved.
Show resolved Hide resolved
if time_range[1] <= time_range[0]:
raise ValueError("time_range[1] must be greater than time_range[0]")
alejoe91 marked this conversation as resolved.
Show resolved Hide resolved

assert mode in ("auto", "line", "map"), 'Mode must be one of "auto","line", "map"'
if mode == "auto":
if len(channel_ids) <= 64:
Expand Down
Loading