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

Round instead of int for time_to_sample_index. #3119

Conversation

JoeZiminski
Copy link
Collaborator

@JoeZiminski JoeZiminski commented Jul 1, 2024

In BaseRecordingSegment.time_to_sample_index() the calculation from time was cast from float to int. By default this will use floor method of rounding, which can lead to wrong results e.g. doing:

sample = 500
time_ = recording.sample_index_to_time(sample)
sample_again = recording.time_to_sample_index(time_)

Could lead to sample not equalling sampling_again.

This PR is tested in #3118.

@JoeZiminski JoeZiminski changed the title Round instead of int for 'time_to_sample_index'. Round instead of int for time_to_sample_index. Jul 1, 2024
@alejoe91 alejoe91 added this to the 0.101.0 milestone Jul 2, 2024
@alejoe91 alejoe91 added the core Changes to core module label Jul 2, 2024
Copy link
Collaborator

@h-mayorquin h-mayorquin left a comment

Choose a reason for hiding this comment

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

LGTM. I also think that at some point we should vectorize these functions as discussed in #3029

@samuelgarcia samuelgarcia merged commit 30286c0 into SpikeInterface:main Jul 3, 2024
17 checks passed
@zm711
Copy link
Collaborator

zm711 commented Jul 7, 2024

I just learned a fun fact about rounding in python

>>> round(4.5)
4

Python tries to reduce bias in rounding and using the .5 rule actual introduces an upward bias. (I think @h-mayorquin and @JoeZiminski would be most interested in this).

@JoeZiminski
Copy link
Collaborator Author

Cool @zm711 I did not know about that!! They taught us wrong in school!

@h-mayorquin
Copy link
Collaborator

I don't remember that they taught us the rounding up rule in school so maybe our system was different.

I actually never read about how rounding works in python and the different rules, thanks Zach for this rabbit hole. Check this out:

In [12]: [i + .5 for i in range(-5, 5)]
Out[12]: [-4.5, -3.5, -2.5, -1.5, -0.5, 0.5, 1.5, 2.5, 3.5, 4.5]

In [13]: [round(i + .5) for i in range(-5, 5)]
Out[13]: [-4, -4, -2, -2, 0, 0, 2, 2, 4, 4]

I wonder if @DradeAW is aware of this as I remember we discussed some of this behavior when changing int to round at some point.

@zm711
Copy link
Collaborator

zm711 commented Jul 9, 2024

It does raise the question of expectation vs the choice to reduce bias. I would have predicted that round followed the rule that Joe and I learned in school as a default rather than trying to eliminate biases. So round is less consistent than int unless we change the rules ourselves. But It is super cool. Definitely would love to hear what @DradeAW thinks. I also can't remember if we do any np.round here or in Neo and whether the behaviors are the same.

@DradeAW
Copy link
Contributor

DradeAW commented Jul 9, 2024

I was not aware of that, this is super weird!
I was always taught to round away from 0 when it was perfectly x.5

Do you think having exactly .5 happens in real cases?

@h-mayorquin
Copy link
Collaborator

Behavior of np.round is the same:

In [19]: [np.round(i + .5) for i in range(-5, 5)]
Out[19]: [-4.0, -4.0, -2.0, -2.0, -0.0, 0.0, 2.0, 2.0, 4.0, 4.0]

In [20]: [i + .5 for i in range(-5, 5)]
Out[20]: [-4.5, -3.5, -2.5, -1.5, -0.5, 0.5, 1.5, 2.5, 3.5, 4.5]

@DradeAW I don't think so, and my hunch is that if a behavior depends on how do you round 0.5 that's ... kind of wrong or you have too few samples anyway?

@zm711
Copy link
Collaborator

zm711 commented Jul 9, 2024

My prediction is that all of our schools just taught a simple binary decision because that is easy for humans. Knowing that .5 rounds up for some numbers and down for others so it hard for a person to keep track of. For a computer we can actually implement sophisticated rules to try to do this statistically. But that's just my guess. I haven't checked but I heard there are multiple implementations possible for rounding in general all with pros and cons (typically speed vs bias).

@DradeAW
Copy link
Contributor

DradeAW commented Jul 9, 2024

I think for float, 0.5 is almost never going to happen, unless you have a specific conversion to µV (in which case it could be frequent).

But I think this problem is less problematic than int() which doubles the amount of zeros ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Changes to core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants