Fix toString(base) under-rounding for odd bases (#412) - #413
Open
youdie006 wants to merge 1 commit into
Open
Conversation
toString(base) under-rounded for odd bases: under DECIMAL_PLACES 0 / ROUND_HALF_UP,
new BigNumber('0.6').toString(3) gave '0' (should be '1') and '2.6'.toString(3) gave
'2' (should be '10'), while .toString(10) is correct.
In convertBase the round-up decision compared the single rounding digit against
k = baseOut/2, which is fractional for odd bases, so the i == k branch was
unreachable and the remainder flag was never consulted. A local tweak is
insufficient: for an odd base half a ULP is 0.mmm... (m = (baseOut-1)/2), so the
tail must be compared against a repeating m, but div only produced ~2 guard digits.
Fix: (1) for an odd output base from toString, compute enough extra base-baseOut
digits (bounded by fracLen*log_baseOut(10)) that the first tail digit differing from
m is available; (2) replace the round-up decision with a base-parity-aware one -
directed modes unchanged, even bases byte-identical, odd bases compare the tail
digit-by-digit against m, with HALF_EVEN using even-mantissa (even sum of kept
digits, since baseOut is odd). Even-base and decimal output are unchanged.
Fixes MikeMcl#412
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.
Fixes #412.
Problem
toString(base)under-rounds for odd bases. Under{ DECIMAL_PLACES: 0, ROUNDING_MODE: ROUND_HALF_UP }:.toString(10)is correct, so base 10 is the oracle. The values are exact (string constructor), so this is not float error.Root cause
In
convertBasethe round-up decision compared the single rounding digitiagainstk = baseOut / 2. For an oddbaseOut,kis fractional (1.5 for base 3), soi == kis unreachable and the remainder flag is never consulted: a discarded tail whose leading digit isfloor(baseOut/2)was treated as "< half" even when the full tail was >= half a ULP.A local tweak isn't sufficient: for an odd base, half a ULP is
0.mmm...(m = (baseOut-1)/2), a non-terminating fraction, so the tail must be compared against a repeating m -- butdivonly produced ~2 guard digits, giving byte-identical output for values that must round differently (e.g. base-30.49->0 vs0.51->1 both yield[1,1,1]).Fix
baseOutdigits that the first tail digit differing from m is always available. The leading run of m digits before the tail is decided is at mostfracLen * log_baseOut(10).xc[d-1] & 1test.Even-base and decimal output are unchanged.
Tests
dist/was rebuilt withnode build.js.This fix was prepared with AI assistance (Claude) and reviewed by me before submission.