fix: compute LCM with integer arithmetic to avoid float precision loss and overflow#810
Open
gaoflow wants to merge 1 commit into
Open
fix: compute LCM with integer arithmetic to avoid float precision loss and overflow#810gaoflow wants to merge 1 commit into
gaoflow wants to merge 1 commit into
Conversation
…s and overflow Lcm previously computed |a*b|/gcd via float64, which fails in two ways: - int64 multiply overflow: a*b wraps mod 2^64 before the float64 cast, so e.g. Lcm(4294967296, 8589934592) returned 0 (impossible for positive inputs) and Lcm(3037000500, 3037000500) returned 3037000499, violating lcm(x,x)==x. - float64 precision loss: float64 cannot represent every int64 past 2^53, so e.g. Lcm(9007199254740993, 1) returned 9007199254740992, violating the identity lcm(x,1)==x, and Lcm(1000000007, 1000000009) was off by 63. Replace the float64 formula with integer arithmetic that divides |a| by gcd before multiplying by |b|, so the running intermediate never exceeds the true LCM and no float conversion occurs. Add regression tests covering both modes while preserving the existing test cases.
Not up to standards ⛔
|
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.
Summary
math/lcm.Lcmroutes its entire computation throughfloat64, which producessilently wrong results for inputs that are not particularly large. The most
basic LCM identity is violated:
lcm(x, 1) == xis violated (no external oracle needed)Lcm(9007199254740993, 1)returns9007199254740992— a value smaller thanits first input.
9007199254740993is2^53 + 1, which is not exactlyrepresentable in
float64, so theint64 -> float64 -> int64round trip dropsthe low bit.
lcm(x, 1)must equalx; this needs no reference implementationto see it is wrong.
lcmof two positives returns0Lcm(4294967296, 8589934592)(i.e.2^32and2^33) returns0. The LCM oftwo positive integers cannot be
0. Herea*bis anint64multiply evaluatedbefore the
float64(...)cast, so it wraps mod2^64when|a*b| >= 2^63;the true LCM (
2^33) fits comfortably inint64, but the intermediate productoverflows first.
Further cases (verified against an exact
math/big.Intoracle)An independent oracle using
math/big.Int(lcm = (|a|/gcd)*|b|, divide first,exact bignum — no third-party LCM/GCD library) confirms:
big.Int)Lcm(4294967296, 8589934592)08589934592Lcm(3037000500, 3037000500)30370004993037000500(lcm(x,x)==x)Lcm(9007199254740993, 1)90071992547409929007199254740993(lcm(x,1)==x)Lcm(1000000007, 1000000009)10000000160000000001000000016000000063(off by 63)Controls are unaffected:
Lcm(12,18)=36,Lcm(28,35)=140,Lcm(4503599627370497,2)=9007199254740994,Lcm(104729,104743)=10969629647.The existing tests (
(1,5),(2,5),(10,5),(5,5)) only covera*b < 2^53with nooverflow, so they pass today (latent gap).
Root cause
Two independent failure modes, both from the
float64detour:int64overflow:a*bwraps mod2^64before thefloat64cast, so the value passed tomath.Absis already wrong even whenthe true LCM fits in
int64.float64precision loss:float64has 53 bits of significand, so itcannot represent every
int64; once|a*b|(or|a|itself) exceeds2^53the round trip drops low bits.
The GCD helper (
math/gcd.Iterative) is correct and is left unchanged; thedefect is purely the
float64routing of the LCM formula.Fix
Replace the
float64formula with integer arithmetic that divides|a|bygcdbefore multiplying by
|b|, so the running intermediate never exceeds thetrue LCM and no
floatconversion occurs:Dividing first is safe because
gdividesaexactly, so|a|/gis an integerno larger than
|a|, and the product equals the true LCM. Ag == 0guardhandles
gcd.Iterative(0, 0)(LCM(0, x)is conventionally0). Minimal,stdlib-only, style-matching the existing code.
Tests
math/lcm/lcm_test.gois extended (table-driven, repo style) with regressioncases, while preserving the existing four:
Lcm(4294967296, 8589934592) == 8589934592(Mode A overflow)Lcm(3037000500, 3037000500) == 3037000500(Mode Alcm(x,x)==x)Lcm(9007199254740993, 1) == 9007199254740993(Mode B2^53+1)Lcm(1000000007, 1000000009) == 1000000016000000063(Mode B precision)All pre-fix these returned the wrong values shown above; post-fix they pass.
Verification
go test ./math/lcm/ ./math/gcd/— PASS (existing + new tests)go test ./math/...— PASS;go build ./...— cleango vet ./math/lcm/ ./math/gcd/— cleangofmt -l math/lcm/lcm.go math/lcm/lcm_test.go— cleanThe change is confined to
math/lcm/;math/gcdis untouched.