Make factor work for Int128, UInt128, BigInt

[JohnAAbbott]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.92%. Comparing base (fd3ff47) to head (8f143f9).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1951   +/-   ##
=======================================
  Coverage   87.92%   87.92%           
=======================================
  Files          99       99           
  Lines       36404    36416   +12     
=======================================
+ Hits        32007    32018   +11     
- Misses       4397     4398    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[thofma]

Suggested change

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

this should work

[JohnAAbbott]

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

[fingolfin]

@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?

[fingolfin]

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?)

[fingolfin]

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.

[JohnAAbbott]

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

[thofma]

I think some .fac calls were missed 8f143f9.

@JohnAAbbott can you open a new PR to fix this? The test should test the interface and not internals.

[fingolfin]

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).

[JohnAAbbott]

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