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

Force original_end_frame >= original_start_frame in tracepad #1886

Closed

Conversation

TomBugnon
Copy link
Contributor

Avoids this error in phase_shift (and possibly other places)

  File "/home/tbugnon/projects/shared_sortings/spikeinterface/src/spikeinterface/preprocessing/phase_shift.py", line 87, in get_traces
    traces_chunk, left_margin, right_margin = get_chunk_with_margin(
  File "/home/tbugnon/projects/shared_sortings/spikeinterface/src/spikeinterface/core/recording_tools.py", line 280, in get_chunk_with_margin
    traces_chunk2 = np.zeros((full_size, traces_chunk.shape[1]), dtype=dtype)
ValueError: negative dimensions are not allowed

@alejoe91
Copy link
Member

@TomBugnon this error also appears for concatenated recordings, right?

@alejoe91 alejoe91 added enhancement New feature or request core Changes to core module bug Something isn't working and removed enhancement New feature or request labels Jul 27, 2023
@TomBugnon
Copy link
Contributor Author

@alejoe91 I got this on a concatenated neuropixel recording (with full preprocessing)
I haven't tried on a simple recording, should I?

@alejoe91
Copy link
Member

No no, I think it should work with simple data!

Comment on lines +122 to +124
original_end_frame = max(
original_end_frame, original_start_frame
) # Avoid negative dimensions errors downstream
Copy link
Member

Choose a reason for hiding this comment

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

I have the feelign that this is hidding a deeper bug.
How we could have this.
Would it be better to raise an error instead no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's a real life scenario where original_end_frame < original_start_frame in TracePad

#1881 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I do think there is an unrelated bug in get_chunk_with_margins , tho): #1885 (comment)

@h-mayorquin
Copy link
Collaborator

h-mayorquin commented Sep 13, 2023

Sam wanted to look at this so I am just writing this for us as a reminder.

Also, we are discussing this issue in more generality here:
#1989
Feedback is welcomed.

@alejoe91
Copy link
Member

@TomBugnon sorry for getting back so late on this.

This issue is gone with #1883 right? In that case I'll close this. Feel free to reopen if that's not the case!

@alejoe91 alejoe91 closed this Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core Changes to core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants