Skip to content

Conversation

tirthbhimani29
Copy link

Problem: The original code crashed (InexactError) when trying to convert a negative Int8 to an UInt16 because unsigned types can't hold negative values.

Solution: my fix avoids this by working with absolute values of the inputs. It uses a larger, signed type (Int128 or BigInt) for all intermediate calculations, preventing both the conversion error and potential overflows of the Bézout coefficients.

Result: The new gcdx is correct and reliable. It calculates the GCD and coefficients on absolute values and then adjusts the signs of the final coefficients to match the original inputs. This ensures it always returns the correct result and maintains the gcdx(a, b)[1] == gcd(a, b) invariant.

@oscardssmith
Copy link
Member

looks like all the tests fail.

@vtjnash
Copy link
Member

vtjnash commented Sep 5, 2025

I assume this meant to reference #58025?

@adienes
Copy link
Member

adienes commented Sep 5, 2025

I think all that is needed are methods like this

gcd(a::Unsigned, b::Signed) = gcd(promote(a, uabs(b))...)
gcd(a::Signed, b::Unsigned) = gcd(b, a)

...


function gcdx(a::Unsigned, b::Signed)
    T = promote_type(signed(typeof(a)), typeof(b))
    return gcdx(T(a), T(b))
end
gcdx(a::Signed, b::Unsigned) = gcdx(b, a)

and actually lcm has a similar issue I believe

lcm(a::Unsigned, b::Signed) = lcm(promote(a, uabs(b))...)
lcm(a::Signed, b::Unsigned) = lcm(b, a)

some tests:

@testset "gcd/gcdx correspondence with mixed signed/unsigned types" begin
    cases = [
        (UInt16(100), Int8(-101)),
        (Int8(-50), UInt16(75)),
        (UInt32(12), Int16(-18)),
        (Int64(-24), UInt8(36)),
        (UInt8(15), Int16(-25)),
        (Int32(-42), UInt64(56)),
        (UInt128(1000), Int32(-1500)),
        (Int8(-128), UInt16(256)),
        (UInt64(0), Int32(-5)),
        (Int16(-7), UInt8(0)),
    ]

    for (a, b) in cases
        g1 = gcd(a, b)
        g2, s, t = gcdx(a, b)
        @test g1 == g2
        @test s*a + t*b == g2
        @test g2 >= 0
    end
end

@tirthbhimani29 tirthbhimani29 closed this by deleting the head repository Sep 10, 2025
adienes pushed a commit that referenced this pull request Sep 25, 2025
Add `gcdx(a::Signed, b::Unsigned)` and `gcdx(a::Unsigned, b::Signed)`
methods to fix #58025:
```julia
julia> gcdx(UInt16(100), Int8(-101))  # pr
(0x0001, 0xffff, 0xffff)

julia> gcdx(UInt16(100), Int8(-101))  # master, incorrect result
(0x0005, 0xf855, 0x0003)
```

Also add the equivalent methods for `lcm` to fix the systematic
`InexactError` when one argument is a negative `Signed` and the other is
any `Unsigned`:
```julia
julia> lcm(UInt16(100), Int8(-101))  # pr
0x2774

julia> lcm(UInt16(100), Int8(-101))  # master, error
ERROR: InexactError: trunc(UInt16, -101)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::UInt16, b::Int8)
   @ Base ./intfuncs.jl:152
 [9] top-level scope
   @ REPL[62]:1
```

Inspired by
#59487 (comment).
The difference is that the solution proposed in this PR keeps the
current correct result type for inputs such as `(::Int16, ::UInt8)`.
KristofferC pushed a commit that referenced this pull request Sep 30, 2025
Add `gcdx(a::Signed, b::Unsigned)` and `gcdx(a::Unsigned, b::Signed)`
methods to fix #58025:
```julia
julia> gcdx(UInt16(100), Int8(-101))  # pr
(0x0001, 0xffff, 0xffff)

julia> gcdx(UInt16(100), Int8(-101))  # master, incorrect result
(0x0005, 0xf855, 0x0003)
```

Also add the equivalent methods for `lcm` to fix the systematic
`InexactError` when one argument is a negative `Signed` and the other is
any `Unsigned`:
```julia
julia> lcm(UInt16(100), Int8(-101))  # pr
0x2774

julia> lcm(UInt16(100), Int8(-101))  # master, error
ERROR: InexactError: trunc(UInt16, -101)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::UInt16, b::Int8)
   @ Base ./intfuncs.jl:152
 [9] top-level scope
   @ REPL[62]:1
```

Inspired by
#59487 (comment).
The difference is that the solution proposed in this PR keeps the
current correct result type for inputs such as `(::Int16, ::UInt8)`.

(cherry picked from commit 4f1e471)
KristofferC pushed a commit that referenced this pull request Sep 30, 2025
Add `gcdx(a::Signed, b::Unsigned)` and `gcdx(a::Unsigned, b::Signed)`
methods to fix #58025:
```julia
julia> gcdx(UInt16(100), Int8(-101))  # pr
(0x0001, 0xffff, 0xffff)

julia> gcdx(UInt16(100), Int8(-101))  # master, incorrect result
(0x0005, 0xf855, 0x0003)
```

Also add the equivalent methods for `lcm` to fix the systematic
`InexactError` when one argument is a negative `Signed` and the other is
any `Unsigned`:
```julia
julia> lcm(UInt16(100), Int8(-101))  # pr
0x2774

julia> lcm(UInt16(100), Int8(-101))  # master, error
ERROR: InexactError: trunc(UInt16, -101)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::UInt16, b::Int8)
   @ Base ./intfuncs.jl:152
 [9] top-level scope
   @ REPL[62]:1
```

Inspired by
#59487 (comment).
The difference is that the solution proposed in this PR keeps the
current correct result type for inputs such as `(::Int16, ::UInt8)`.

(cherry picked from commit 4f1e471)
KristofferC pushed a commit that referenced this pull request Sep 30, 2025
Add `gcdx(a::Signed, b::Unsigned)` and `gcdx(a::Unsigned, b::Signed)`
methods to fix #58025:
```julia
julia> gcdx(UInt16(100), Int8(-101))  # pr
(0x0001, 0xffff, 0xffff)

julia> gcdx(UInt16(100), Int8(-101))  # master, incorrect result
(0x0005, 0xf855, 0x0003)
```

Also add the equivalent methods for `lcm` to fix the systematic
`InexactError` when one argument is a negative `Signed` and the other is
any `Unsigned`:
```julia
julia> lcm(UInt16(100), Int8(-101))  # pr
0x2774

julia> lcm(UInt16(100), Int8(-101))  # master, error
ERROR: InexactError: trunc(UInt16, -101)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::UInt16, b::Int8)
   @ Base ./intfuncs.jl:152
 [9] top-level scope
   @ REPL[62]:1
```

Inspired by
#59487 (comment).
The difference is that the solution proposed in this PR keeps the
current correct result type for inputs such as `(::Int16, ::UInt8)`.

(cherry picked from commit 4f1e471)
xal-0 pushed a commit to xal-0/julia that referenced this pull request Sep 30, 2025
Add `gcdx(a::Signed, b::Unsigned)` and `gcdx(a::Unsigned, b::Signed)`
methods to fix JuliaLang#58025:
```julia
julia> gcdx(UInt16(100), Int8(-101))  # pr
(0x0001, 0xffff, 0xffff)

julia> gcdx(UInt16(100), Int8(-101))  # master, incorrect result
(0x0005, 0xf855, 0x0003)
```

Also add the equivalent methods for `lcm` to fix the systematic
`InexactError` when one argument is a negative `Signed` and the other is
any `Unsigned`:
```julia
julia> lcm(UInt16(100), Int8(-101))  # pr
0x2774

julia> lcm(UInt16(100), Int8(-101))  # master, error
ERROR: InexactError: trunc(UInt16, -101)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int8)
   @ Core ./boot.jl:866
 [2] check_sign_bit
   @ ./boot.jl:872 [inlined]
 [3] toUInt16
   @ ./boot.jl:958 [inlined]
 [4] UInt16
   @ ./boot.jl:1011 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:379 [inlined]
 [7] promote
   @ ./promotion.jl:404 [inlined]
 [8] lcm(a::UInt16, b::Int8)
   @ Base ./intfuncs.jl:152
 [9] top-level scope
   @ REPL[62]:1
```

Inspired by
JuliaLang#59487 (comment).
The difference is that the solution proposed in this PR keeps the
current correct result type for inputs such as `(::Int16, ::UInt8)`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants