-
Notifications
You must be signed in to change notification settings - Fork 190
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
Round instead of int for time_to_sample_index
.
#3119
Conversation
time_to_sample_index
.
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.
LGTM. I also think that at some point we should vectorize these functions as discussed in #3029
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). |
Cool @zm711 I did not know about that!! They taught us wrong in school! |
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. |
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 |
I was not aware of that, this is super weird! Do you think having exactly .5 happens in real cases? |
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? |
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). |
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 |
In
BaseRecordingSegment.time_to_sample_index()
the calculation from time was cast fromfloat
toint
. By default this will use floor method of rounding, which can lead to wrong results e.g. doing:Could lead to
sample
not equallingsampling_again
.This PR is tested in #3118.