sagemath/sage

switch to libgap in sage/coding

Closed this issue · 33 comments

after #34737 there are still uses of pexpect GAP left there.
We remove them on this ticket.

Part of #26902

Depends on #34770

CC: @kwankyu @fchapoton @wdjoyner @mkoeppe

Component: coding theory

Author: Dima Pasechnik

Branch/Commit: b1257bf

Reviewer: Vincent Delecroix, Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/34771

Dependencies: #34770

comment:1

Trying to use #34770 to avoid explict calls to gfq_gap_to_sage from sage.interfaces.gap.

Author: Dima Pasechnik

Commit: 8777160

New commits:

21e38a7fix coersion of libgap FFelements; switch to libgap
d4bde80make imports of is_GapEelment uniform across files
8777160use only libgap in coding/linear_code

Changed commit from 8777160 to b082066

Branch pushed to git repo; I updated commit sha1. New commits:

b082066use libgap in coding/binary_code

Changed commit from b082066 to ec1c72e

Branch pushed to git repo; I updated commit sha1. New commits:

ec1c72efixed off by 1 error: switching to Python's indexing from GAP's
comment:5

2 doctest errors in coding/ with these changes:

File "src/sage/coding/code_constructions.py", line 732, in sage.coding.code_constructions.ToricCode
Failed example:
    C.minimum_distance()
Expected:
    24
Got:
    30
**********************************************************************
File "src/sage/coding/code_constructions.py", line 740, in sage.coding.code_constructions.ToricCode
Failed example:
    C.minimum_distance()
Expected:
    6
Got:
    10

note that doctests using Guava for these codes added in ec1c72e output correct minimal distances.

Description changed:

--- 
+++ 
@@ -1,4 +1,5 @@
 after #34737 there are still uses of pexpect GAP left there.
+We remove them on this ticket.
 
 
 Part of #26902

Branch pushed to git repo; I updated commit sha1. New commits:

249acaado not overwrite all-0 vector v0

Changed commit from ec1c72e to 249acaa

Changed commit from 249acaa to d60bf32

Branch pushed to git repo; I updated commit sha1. New commits:

d60bf32use libgap in doctest
comment:11

With #34391, few doctests related to Guava will need to be adjusted, as Guava is less chatty in GAP 4.12.1.

Branch pushed to git repo; I updated commit sha1. New commits:

274dc61easier way to get Guava's bindir

Changed commit from d60bf32 to 274dc61

comment:13

This is doing way too much conversions

H = libgap(aut_m).Intersection2(aut_B_aug)
rt_transversal = aut_B_aug.RightTransversal(H).List().sage()
rt_transversal = [PermutationGroupElement(g) for g in rt_transversal if str(g) != '()']
rt_transversal = [[a-1 for a in g.domain()] for g in rt_transversal]
rt_transversal = [g + list(xrange(len(g), n))

It would be nicer as

H = libgap(aut_m).Intersection2(aut_B_aug)
rt_transversal = aut_B_aug.RightTransversal(H)
rt_transversal = [[int(a) - 1 for a in g.ListPerm(n)] for g in rt_transversal if not g.IsOne()]
comment:14

At the time guava is loaded, one should check for its availability. The standard way is

sage: from sage.features.gap import GapPackage
sage: pkg = GapPackage("guava")
sage: pkg.require()
Traceback (most recent call last):
...
FeatureNotPresentError: gap_package_guava is not available.
`TestPackageAvailability("guava")` evaluated to `fail` in GAP.

Reviewer: Vincent Delecroix

comment:16

Replying to Vincent Delecroix:

This is doing way too much conversions

H = libgap(aut_m).Intersection2(aut_B_aug)
rt_transversal = aut_B_aug.RightTransversal(H).List().sage()
rt_transversal = [PermutationGroupElement(g) for g in rt_transversal if str(g) != '()']
rt_transversal = [[a-1 for a in g.domain()] for g in rt_transversal]
rt_transversal = [g + list(xrange(len(g), n))

It would be nicer as

H = libgap(aut_m).Intersection2(aut_B_aug)
rt_transversal = aut_B_aug.RightTransversal(H)
rt_transversal = [[int(a) - 1 for a in g.ListPerm(n)] for g in rt_transversal if not g.IsOne()]

Sure, thanks, I agree. My goal was to get it to work with libgap rather than pexpect gap, I mostly went with tweaking pre-existing code line by line.

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

1c4a69fuse GapElement from an abstract superclass
6e5ea79move import of libgap into a function
ee9a3d4use only libgap in coding/linear_code
1f67fa2use libgap in coding/binary_code
7528cfbfixed off by 1 error: switching to Python's indexing from GAP's
2bdf591do not overwrite all-0 vector v0
4167b6duse libgap in doctest
5e463c8easier way to get Guava's bindir
781677fadd missing string conversion
8b0526ecode simplification, checking for Guava availability

Changed commit from 274dc61 to 8b0526e

comment:18

fixed, rebased over the latest version of #34770. Ready for review.

comment:20

ping?

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

730cd62use only libgap in coding/linear_code
5232c56use libgap in coding/binary_code
930eea6fixed off by 1 error: switching to Python's indexing from GAP's
6576c4cdo not overwrite all-0 vector v0
df9628cuse libgap in doctest
318b431easier way to get Guava's bindir
6513816add missing string conversion
b1257bfcode simplification, checking for Guava availability

Changed commit from 8b0526e to b1257bf

comment:22

please review

Changed reviewer from Vincent Delecroix to Vincent Delecroix, Matthias Koeppe

comment:23

LGTM

Changed branch from u/dimpase/coding/use_libgap to b1257bf