Improve conversion between OSCAR and GAP prime fields

[fingolfin]

Before:

julia> hom = Oscar._iso_oscar_gap(GF(2));

julia> e = one(domain(hom)); @btime hom(e);
  2.222 μs (21 allocations: 736 bytes)

julia> e = one(codomain(hom)); @btime preimage(hom, e);
  1.733 μs (17 allocations: 624 bytes)

After:

julia> hom = Oscar._iso_oscar_gap(GF(2));

julia> e = one(domain(hom)); @btime hom(e);
  154.096 ns (3 allocations: 48 bytes)

julia> e = one(codomain(hom)); @btime preimage(hom, e);
  232.591 ns (2 allocations: 96 bytes)

[ThomasBreuer]

If FO is Nemo.FpField(p) with large enough p then e and y*e will not have the type GAP.FFE.
(They will be in the GAP filter IsFFE but not in ÌsInternalRep.)

[fingolfin]

Thanks for paying close attention; yes that wasn't meant to be here at all, I only had it in there for an experiment (to show to @fieker that adding a type insertion there causes an extra allocation that I don't understand) and then accidentally committed it :-(.

Removed now.

[ThomasBreuer]

I think prescribing GAP.FFE will in general not work.
(Some test for this situation seems to be missing ...)

[fingolfin]

I'll not touch the d>1 case for now. Just some notes: I think for good performance we'll need more special cases.

E.g. GAPWrap.Coefficients in the generic finv methods by itself already performs 22 allocations if I consider GF(2,4). It computes powers of the input element and then applies a transformation matrix. In that case it is much faster to use that we have the Zech logarithm, and use something like this:

     z = GAP.Globals.PrimitiveRoot(FG)
     finv_neu = function(x::GAP.FFE) # TODO improve
       return gen(FO)^GAP.Globals.LogFFE(x, z)
     end

Conversely, if the input is an FqField (which it probably is most of the time these days), then I think we need code that distinguishes which kind of FqField it is, and e.g. if it is also given by Zech logarithms, then make use of that, too.

@fieker suggested among other things that we could change

      finv = function(x::GAP.Obj)
        v = GAPWrap.Coefficients(basis_FG, x)
        v_int = [ZZRingElem(GAPWrap.IntFFE(v[i])) for i = 1:d]
        return sum([v_int[i]*basis_F[i] for i = 1:d])
      end

by replacing the return by return FO(v_int) which sounds good, except it doesn't really improve anything as that constructor takes the int vector and coerces it into a polynomial...

function (a::FqField)(b::Vector{<:IntegerUnion})
   da = degree(a)
   db = length(b)
   da == db || error("Coercion impossible")
   return a(parent(defining_polynomial(a))(b))
end

(I also experimented with changing from ZZRingElem to Int when the field is small enough, which doesn't help at all right now as code in the final return completely dominates)

[thofma]

I have not checked, but if we already have efficient conversions between GAP and the "old" finite fields (aka fpField, FpField, fqPolyRepField, FqPolyRepField), then it should be relatively easy to reuse this for FqField. I am happy to help, if you want to go down this road.

[fieker]

On Tue, Apr 16, 2024 at 12:13:53AM -0700, Tommy Hofmann wrote: I have not checked, but if we already have efficient conversions between GAP and the "old" finite fields (aka `fpField`, `FpField`, `fqPolyRepField`, `FqPolyRepField`), then it is relatively easy to reuse this. I am happy to help, if you want to go down this road.

Part of the point is, we never had - efficient conversion for elements - even less for matrices ctrl-c showed that GF(3, 4) conversion would create a GAPInt Coeff-Vector, then convert it to a Vector{ZZRingElem}, then do a sum(c[i]*b[i] ...) to compute the lin comb. Quite likely, K(Int[]) would have done the job...

…

-- Reply to this email directly or view it on GitHub: #3611 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

[fingolfin]

Quite likely, K(Int[]) would have done the job...

@fieker but note that K(Int[...]) is also very inefficient right now (it first constructs a polynomial, then feeds that to a flint function)

[fingolfin]

@ThomasBreuer I assume you added the conversion to ZZRingElem because we otherwise get a UInt.

We should have a better function to "convert a Julia or Oscar integer to GAP integer" which does the right thing for UInt (i.e. convert it to an Int it if fits and otherwise to a GAP larger integer). A natural way to express that would be via GapInt(x) methods

[ThomasBreuer]

Yes, the other coeff and _coeff methods return ZZRingElem, and here we get a UInt. This is not really a problem if we are sure that anyhow GAP.Obj will be called for the result. Thus perhaps we do not need _ffe_to_int but _ffe_to_gapint?

[fingolfin]

Yeah, and we can use julia_to_gap to get "efficient" conversion for UInt, too (as long as they are less than $2^{60}$ at least; for larger UInt it still first converts to a BigInt. I could optimize that, too, but that's for GAP.jl

[ThomasBreuer]

Yes, julia_to_gap(ZZ(2)) is faster than GAP.Obj(ZZ(2)). (Is this part of the problem or part of the solution?)

(The code for GAP.Obj looks worse than it is. It calls julia_to_gap with an often unnecessary IdDict object, but it looks as if this gets optimized away in calls such as GAP.Obj(ZZ(2)). Or I am misunderstanding what is going on.)

[fieker]

creating a poly is bad, but has no arithmetic costs. Its one stupid allocation, but not a single computation

[fingolfin]

The polynomials are more than one allocation because the coefficients are first converted from Int to ZZRingElem. Consider this:

julia> v = [0,0,0,1,0];

julia> F = GF(2,5);

julia> @time F(v)
  0.000018 seconds (17 allocations: 888 bytes)
o^3

This calls the following method:

function (a::FqField)(b::Vector{<:IntegerUnion})
   da = degree(a)
   db = length(b)
   da == db || error("Coercion impossible")
   return a(parent(defining_polynomial(a))(b))
end

Let's create that polynomial then:

julia> R =parent(defining_polynomial(F))
Univariate polynomial ring in x over GF(2)

julia> @time R(v)
  0.000011 seconds (14 allocations: 776 bytes)
x^3

Well, what is the code here? This:

function (R::FqPolyRing)(x::Vector{T}) where {T <: Integer}
   length(x) == 0 && return zero(R)
   return R(map(ZZRingElem, x))
end

That said, I still think we should use the F(v) syntax for now and then improve the underlying Nemo methods.

But there is more: if the Nemo field internally really uses Zech logarithms, it would be great to know so that we can make use of this. Same for other representations.

[fingolfin]

@ThomasBreuer For now I removed the ZZ(...) from the one _ffe_to_int method and just call GAP.julia_to_gap here, that should work fine for now. On the long run my plan is to use GAP.GapInt instead, see oscar-system/GAP.jl#984

[fingolfin]

@ThomasBreuer OK to merge for now? Then please approve, we can still improve it later

[ThomasBreuer]

Nice, thanks.