Skip to content

Round instead of int for time_to_sample_index. - #3119

Merged
samuelgarcia merged 1 commit into
SpikeInterface:mainfrom
JoeZiminski:use_round_for_time_to_sample_index
Jul 3, 2024
Merged

samuelgarcia merged 1 commit into
SpikeInterface:mainfrom
JoeZiminski:use_round_for_time_to_sample_index

Conversation

@JoeZiminski

@JoeZiminski JoeZiminski commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

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

@h-mayorquin h-mayorquin left a comment

Copy link
Copy Markdown
Contributor

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
@zm711

zm711 commented Jul 7, 2024

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

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

@h-mayorquin

Copy link
Copy Markdown
Contributor

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

zm711 commented Jul 9, 2024

Copy link
Copy Markdown
Member

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

DradeAW commented Jul 9, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

zm711 commented Jul 9, 2024

Copy link
Copy Markdown
Member

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

DradeAW commented Jul 9, 2024

Copy link
Copy Markdown
Contributor

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