Patch GAP package Alnuth to call OSCAR instead of PARI

[fingolfin]

There are still some bugs in it, I'll work on fixing those and adding some proper tests next.

Should be fully functional now.

UPDATE: I've also submitted the changes as a PR to Alnuth, so that its next release will support this out of the box.

[codecov]

Codecov Report

Merging #2220 (8464d2e) into master (138fee2) will increase coverage by 0.00%.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2220   +/-   ##
=======================================
  Coverage   73.43%   73.43%           
=======================================
  Files         367      367           
  Lines       50043    50049    +6     
=======================================
+ Hits        36748    36753    +5     
- Misses      13295    13296    +1     

see 8 files with indirect coverage changes

[fingolfin]

Done in oscar-system/GAP.jl#861

[fingolfin]

@ThomasBreuer any thoughts on this? Otherwise I'll merge this soon

[ThomasBreuer]

Suggested change

	gap> START_TEST("Factorisation of polynomials using PARI/GP");
	gap> START_TEST("Factorisation of polynomials using Oscar");

[ThomasBreuer]

Looks complicated. What is wrong with JuliaToGAP(IsList, g, true)?

[fingolfin]

Huh, good question. I am pretty sure I tried that and there was an error back then, but I probably just did it wrong somehow... It definitely passes the tests now, so I'll use that. Thanks!

[ThomasBreuer]

This looks very nice. (I have added just two minor comments.)

If I understand the comment "also store the isomorphism" for _OscarField right then the point is to support isomorphisms from the number fields defined in alnuth (matrix fields) to number fields in Oscar. In order to provide this in a file in Oscar/src/GAP, we have to assume that the alnuth package is already loaded.
I can provide such an isomorphism in a separate pull request.
(In the direction from Oscar to GAP, iso_oscar_gap creates an AlgebraicExtension field from a number field.)

[fingolfin]

@ThomasBreuer yes that sounds right: we are given an "alnuth number field", and want to store not just an isomorphic Oscar field, but also an isomorphism that can compute images and preimages.

Once that is in place, we can start looking into a larger refactoring of Alnuth: in the current setup, a lot of redundant work is done (e.g. GAP code reverses the order of coefficients for a polynomial, as that is what Pari expects; but we hijack that, and have to undo the reverse; compute; then reverse our answer, only for GAP to then re-reverse....). And there are a bunch of Alnuth functions that could be done much more efficiently if we were to "inject" ourselves at a higher level... But this is future work, the first goal is to have this working without Pari, which this PR achieves.

Next, I'll make a new Alnuth release with much of this code in it. Once GAP 4.13.0 is out, we can simplify code here again.

[ThomasBreuer]

@fingolfin The natural way to define iso_gap_oscar for the fields used in Alnuth would be to create an Oscar field with the same defining polynomial. In fact, such a method is already available for those fields that are created by FieldByPolynomial (which delegates to AlgebraicExtension), thus only a method for the fields of matrices is missing.

However, the current _OscarField is created from the IntegerDefiningPolynomial of the GAP field, not from its DefiningPolynomial; this fits to the function PolynomialFactorsDescriptionPari, which also accesses the IntegerDefiningPolynomial.
(In fact, I do currently not see where the Alnuth code resolves this "twist" in the end.)

My question is: Shall iso_gap_oscar create an Oscar field from the IntegerDefiningPolynomial of the GAP field (and "scale" the field elements when translating them), or shall the DefiningPolynomial of the GAP field be taken, and the Alnuth code that uses the isomorphism has to take care of this fact?