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

spec: round values #636

Merged
merged 5 commits into from
Jun 22, 2023
Merged

spec: round values #636

merged 5 commits into from
Jun 22, 2023

Conversation

JensenPaul
Copy link
Collaborator

@JensenPaul JensenPaul commented Jun 16, 2023

@JensenPaul JensenPaul added the spec Relates to the spec label Jun 16, 2023
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@JensenPaul JensenPaul merged commit d4dea52 into main Jun 22, 2023
1 check passed
@JensenPaul JensenPaul deleted the JensenPaul-patch-2 branch June 22, 2023 18:53
github-actions bot added a commit that referenced this pull request Jun 22, 2023
SHA: d4dea52
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@michaelkleber
Copy link
Collaborator

Hey @JensenPaul, I don't think the rounding algorithm here accomplishes its intended goal. We should round values up or down in such a way that the expected value of the result is the original number. In your steps 6/7/8, if precisionScaledValue is 12.3, then truncatedScaledValue will always be 12, while we want it to come out 13 in 30% of the cases.

Seems like the way to do that is to define the fractional part of precisionScaledValue, and then compare that to a uniformly random value between 0 and 1. If the fractional part is smaller than the random value we round down, while if the fractional part is larger than the random value we round up.

It also seems like the spec should mention the intended mathematical property of the algorithm, so that people can easily see the goal and verify that we're meeting it.

@michaelkleber
Copy link
Collaborator

D'oh, as @brusshamilton just pointed out to me, what I wrote is exactly the same as just changing the random value in step 7 to be between 0 and 1, rather than the current version that is between 0 and 0.5.

spec.bs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants