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.

Sign up for GitHub

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

Make factor work for Int128, UInt128, BigInt #1951

Merged
merged 3 commits into from
Nov 28, 2024
Merged

Make factor work for Int128, UInt128, BigInt #1951

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
744d5bf
Make factor work for Int128, UInt128, BigInt
JohnAAbbott Nov 28, 2024
b0663da
Activate integer factor tests
JohnAAbbott Nov 28, 2024
8f143f9
Neater calls to length
JohnAAbbott Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/flint/fmpz.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2894,6 +2894,7 @@ end
(::Type{T})(a::ZZRingElem) where T <: Union{UInt8, UInt16, UInt32} = T(UInt(a))

(::Type{Int128})(a::ZZRingElem) = Int128(BigInt(a))
(::Type{UInt128})(a::ZZRingElem) = UInt128(BigInt(a))

Integer(a::ZZRingElem) = BigInt(a)

Expand Down
23 changes: 21 additions & 2 deletions src/flint/fmpz_factor.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
###############################################################################
#
# Flint factor(s) functiosn
# Flint factor(s) functions
#
###############################################################################

Expand All @@ -17,20 +17,39 @@ function _factor(a::ZZRingElem)
return res, canonical_unit(a)
end

# Just to give a helpful error message if someone tries to factor a boolean
function factor(b::Bool)
throw(DomainError("Cannot factorize a boolean"));
end

# This function handles machine integer types (up to 64 bits)
function factor(a::T) where T <: Union{Int, UInt}
iszero(a) && throw(ArgumentError("Argument must be non-zero"))
u = sign(a)
a = u < 0 ? -a : a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is actually incorrect if a == typemin(Int), as then -a == a and we get an error casting that negative value to UInt.

But instead of fixing this locally, I suggest a slightly bigger change which then also allows avoiding creating Dictionaries twice:

Change this function to look like roughly this (might contain syntax errors):

function factor(::Type{T}, a::UInt) where T <: Integer
  iszero(a) && throw(ArgumentError("Argument must be non-zero"))
  F = n_factor()
  # ... now continue as before
end

factor(a::UInt) = factor(UInt, a)

I.e. the caller can specify T.

Then continue:

# another base cased, needed because `flintify` may produce `Int`
function factor(::Type{T}, a::Int) where T <: Integer
  u = reinterpret(UInt, a < 0 ? -a : a)
  return factor(T, u)
end

function factor(a::T) where T <: Integer
  return factor(T, flintify(a))
end

function factor(::Type{T}, N::ZZRingElem) where T <: Integer
  F = factor(N)
  res = Dict{T, Int}()  # factor-multiplicity pairs
  for (fac,exp) in F.fac
    res[T(fac)] = exp
  end
  return Fac(u, res)
end

Of course the final function is also not optimal, better would be to modify function factor(N::ZZRingElem) to take T as argument and use it, but that could be done later (in any case this is no worse than what your code right now).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided not to alter factor(::ZZRingElem) whose implementation is "special" (and possibly fragile?).

F = n_factor()
@ccall libflint.n_factor(F::Ref{n_factor}, a::UInt)::Nothing
res = Dict{T, Int}()
res = Dict{T, Int}() # factor-multiplicity pairs
for i in 1:F.num
z = F.p[i]
res[z] = F.exp[i]
end
return Fac(u, res)
end

# This is supposed to be called only for T in [Int128, UInt128, BigInt]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand this comment. The code clearly can also be called for e.g. T equal to Int16. So who is supposed to "only" call it for the listed types? What happens if I call it with another Julia Integer type? Is there a problem (then we should address it)? Or is actually all fine (then remove this comment?)

function factor(a::T) where T <: Integer
iszero(a) && throw(ArgumentError("Argument must be non-zero"))
u = sign(a)
F = factor(ZZ(abs(a)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing you could try in a follow-up PR: if T is an Integer type that is "smaller" than Int or UInt, this might be more efficient:

Suggested change
F = factor(ZZ(abs(a)))
F = factor(flintify(abs(a)))

Check out the docstring for Nemo.flintify to get an explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint. Note that flintify seems always to give a ZZRingElem when the supplied value is Unsigned; is this intentional?

res = Dict{T, Int}() # factor-multiplicity pairs
for (fac,exp) in F.fac
res[T(fac)] = exp
end
return Fac(u, res)
end


################################################################################
#
# ECM
Expand Down
1 change: 1 addition & 0 deletions test/Rings-test.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const ring_to_mat = Dict(ZZ => ZZMatrix,
)

include("flint/fmpz-test.jl")
include("flint/fmpz_factor-test.jl")
include("flint/fmpz_poly-test.jl")
include("flint/fmpz_mod_poly-test.jl")
include("flint/gfp_fmpz_poly-test.jl")
Expand Down
59 changes: 59 additions & 0 deletions test/flint/fmpz_factor-test.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@

@testset "Integer factorization" begin

@test_throws ArgumentError factor(0)
@test_throws ArgumentError factor(Int128(0))
@test_throws ArgumentError factor(UInt128(0))
@test_throws ArgumentError factor(BigInt(0))
@test_throws ArgumentError factor(ZZ(0))


@test_throws DomainError factor(true)
@test_throws DomainError factor(false)


# trivial case: input is 1
F1 = factor(1)
F1_ZZ = factor(ZZ(1))
@test length(F1.fac) == 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@test length(F1.fac) == 0
@test length(F1) == 0

this should work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for the suggested clean up. I have just checked in the relevant changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnAAbbott Maybe you checked them in but did not push them? They don't appear to be in the merged code. Perhaps send a follow-up PR to clean that up?

@test length(F1_ZZ.fac) == 0
@test unit(F1) == 1
@test unit(F1_ZZ) == 1


# Test different integer types
F = factor(123456789)
F_Int128 = factor(Int128(123456789))
F_UInt128 = factor(UInt128(123456789))
F_BigInt = factor(BigInt(123456789))
F_ZZ = factor(ZZ(123456789))

@test length(F.fac) == 3
@test length(F_Int128.fac) == 3
@test length(F_UInt128.fac) == 3
@test length(F_BigInt.fac) == 3
@test length(F_ZZ.fac) == 3

@test unit(F) == 1
@test unit(F_Int128) == 1
@test unit(F_UInt128) == 1
@test unit(F_BigInt) == 1
@test unit(F_ZZ) == 1


# an example with two "large" factors
repunit37 = ZZ(10)^37 -1
repunit41 = ZZ(10)^41 -1
FF = factor(repunit37*repunit41) # trickier but still quick
@test length(FF.fac) == 8


# negative input
F_minus1 = factor(-1)
F_minus1_ZZ = factor(ZZ(-1))
@test length(F_minus1.fac) == 0
@test length(F_minus1_ZZ.fac) == 0
@test unit(F_minus1) == -1
@test unit(F_minus1_ZZ) == -1

end
Loading