-
Notifications
You must be signed in to change notification settings - Fork 202
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
Renaming: from_time_labels
-> from_samples_labels
#3724
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, and I agree it would be easier to understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is clutch for understandability. I think the other longer term thing we should think about (pre-1.0 in my opinion) is moving from the frame -> sample terminology in all places.
@@ -711,7 +711,7 @@ In this example, we create a recording and a sorting object from numpy objects: | |||
spike_trains += spike_trains_i | |||
labels += labels_i | |||
|
|||
sorting_memory = NumpySorting.from_times_labels(times=spike_trains, labels=labels, | |||
sorting_memory = NumpySorting.from_samples_labels(times=spike_trains, labels=labels, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How comes the argument is still called times here, is this something we should change?
Two request:
I find the lack of and very confusing in how I read the naming. As in, if the labels were the labels of the times. As I am writing this I am thinking, aren't the labels the unit_ids? Shouldn't we use that terminology? |
I think those are great points Heberto! I was thinking something similar to point 2, but didn't want the double break. But I think you're right that if this breaking change is being done might as well as make it as clear as possible moving forward. I like the |
I like the idea but we need to be backward compatible here (with a warning) |
and adding
from_times_labels
fixes #3723