fix: round ties towards +Infinity in roundTo to match round()#94
Open
spokodev wants to merge 1 commit into
Open
fix: round ties towards +Infinity in roundTo to match round()#94spokodev wants to merge 1 commit into
spokodev wants to merge 1 commit into
Conversation
`roundTo` rounded an exact half by magnitude (half away from zero), while
`round` rounds half towards +Infinity (mirroring Math.round, as documented).
Because rounding to the nearest multiple of 1 is the same as integer
rounding, `x.round()` and `x.roundTo(1)` must agree, but they disagreed on
every negative half:
new Fraction(-1, 2).round() // 0
new Fraction(-1, 2).roundTo(1) // -1 (now 0)
new Fraction(-3, 2).round() // -1
new Fraction(-3, 2).roundTo(1) // -2 (now -1)
Apply the same tie-breaking `round` uses: only round a half up when the
value is non-negative, so the half is broken towards +Infinity regardless
of the multiple.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
roundToandroundbreak exact-half ties differently.roundrounds half towards +Infinity (mirroringMath.round, as the docs say), butroundTorounds the magnitude (half away from zero). Since rounding to the nearest multiple of1is the same as integer rounding,x.round()andx.roundTo(1)must agree — yet they disagree on every negative half:Non-tie values and all positive values were already correct; only negative exact halves were affected.
Cause
roundTocomputesk = floor(n / d)on the unsigned magnitudes and bumpskon a tie withif (r + r >= d) k++, then re-applies the sign. Because the half is resolved on the magnitude, ties always round away from zero.Fix
Use the same sign-aware tie-break that
roundalready uses — round a half up only when the value is non-negative — so ties always break towards +Infinity regardless of the multiple:Tests
Added negative-tie cases to
tests/fraction.test.js(-1/2,-3/2to a multiple of1;-1/4to a multiple of1/2) — red before, green after. Full suite green (317 passing).dist/rebuilt to match (only the touched logic; no unrelated build churn).