kernel: fix wrong result bugs in partial perms code

Various computations involving T_PERM2 objects of degree 65536 returned incorrect results. The reason was that the degree, which is always an UInt, was stored in an UInt2. But for degree 65536, this overflows to zero, hence the code effectively treated the permutation as the identity. The fix is to always store the degree as UInt, not as UInt2 or UInt4 (the latter case actually is probably fine, but let's do it right anyway). The fix in PowPPerm2Perm2 is a bit nasty and constitutes a deoptimization; a proper fix at this stage would involve duplicating and tweaking a bunch of code; but I plan to rewrite this file to use C++ templates anyway, at which point the proper fix will come almost for free. So instead of wasting time on a "proper" fix now, let's live with this workaround for now.
gap-system · Jan 23, 2019 · f27cff4 · f27cff4
1 parent e868e29
commit f27cff4
Show file tree

Hide file tree

Showing 2 changed files with 125 additions and 41 deletions.
diff --git a/src/pperm.cc b/src/pperm.cc
@@ -2843,8 +2843,8 @@ Obj ProdPPerm22(Obj f, Obj g)
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM2);
     GAP_ASSERT(TNUM_OBJ(g) == T_PPERM2);
 
-    UInt   deg, degg, i, j, rank;
-    UInt2 *ptf, *ptg, *ptfg, codeg;
+    UInt   deg, degg, i, j, rank, codeg;
+    UInt2 *ptf, *ptg, *ptfg;
     Obj    fg, dom;
 
     if (DEG_PPERM2(g) == 0)
@@ -2901,9 +2901,9 @@ Obj ProdPPerm42(Obj f, Obj g)
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM4);
     GAP_ASSERT(TNUM_OBJ(g) == T_PPERM2);
 
-    UInt    deg, degg, i, j, rank;
+    UInt    deg, degg, i, j, rank, codeg;
     UInt4 * ptf;
-    UInt2 * ptg, *ptfg, codeg;
+    UInt2 * ptg, *ptfg;
     Obj     fg, dom;
 
     if (DEG_PPERM2(g) == 0)
@@ -3288,9 +3288,9 @@ Obj ProdPPerm4Perm2(Obj f, Obj p)
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM2);
 
     UInt4 *ptf, *ptfp;
-    UInt2 *ptp, dep;
+    UInt2 *ptp;
     Obj    fp, dom;
-    UInt   codeg, deg, i, j, rank;
+    UInt   codeg, deg, dep,i, j, rank;
 
     deg = DEG_PPERM4(f);
     fp = NEW_PPERM4(deg);
@@ -3327,8 +3327,8 @@ Obj ProdPerm2PPerm2(Obj p, Obj f)
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM2);
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM2);
 
-    UInt2 deg, *ptp, *ptf, *ptpf;
-    UInt  degf, i;
+    UInt2 *ptp, *ptf, *ptpf;
+    UInt  deg, degf, i;
     Obj   pf;
 
     if (DEG_PPERM2(f) == 0)
@@ -3370,8 +3370,8 @@ Obj ProdPerm4PPerm4(Obj p, Obj f)
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM4);
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM4);
 
-    UInt4 deg, *ptp, *ptf, *ptpf;
-    UInt  degf, i;
+    UInt4 *ptp, *ptf, *ptpf;
+    UInt  deg, degf, i;
     Obj   pf;
 
     if (DEG_PPERM4(f) == 0)
@@ -3413,9 +3413,9 @@ Obj ProdPerm4PPerm2(Obj p, Obj f)
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM4);
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM2);
 
-    UInt4  deg, *ptp;
+    UInt4  *ptp;
     UInt2 *ptf, *ptpf;
-    UInt   degf, i;
+    UInt   deg, degf, i;
     Obj    pf;
 
     if (DEG_PPERM2(f) == 0)
@@ -3609,8 +3609,8 @@ Obj PowPPerm2Perm2(Obj f, Obj p)
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM2);
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM2);
 
-    UInt   deg, rank, degconj, i, j, k, codeg;
-    UInt2 *ptf, *ptp, *ptconj, dep;
+    UInt   deg, dep, rank, degconj, i, j, k, codeg;
+    UInt2 *ptf, *ptp, *ptconj;
     Obj    conj, dom;
 
     deg = DEG_PPERM2(f);
@@ -3622,6 +3622,12 @@ Obj PowPPerm2Perm2(Obj f, Obj p)
     ptp = ADDR_PERM2(p);
     dom = DOM_PPERM(f);
 
+    // FIXME HACK: workaround bug (proper fix will come with
+    // refactoring of this code to use C++ templates)
+    if (dep == 65536) {
+        return PROD(LQUO(p, f), p);
+    }
+
     // find deg of conjugate
     if (deg > dep) {
         degconj = deg;
@@ -3668,9 +3674,9 @@ Obj PowPPerm2Perm4(Obj f, Obj p)
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM2);
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM4);
 
-    UInt    deg, rank, degconj, i, j, k, codeg;
+    UInt    deg, dep, rank, degconj, i, j, k, codeg;
     UInt2 * ptf;
-    UInt4 * ptp, *ptconj, dep;
+    UInt4 * ptp, *ptconj;
     Obj     conj, dom;
 
     deg = DEG_PPERM2(f);
@@ -3717,8 +3723,8 @@ Obj PowPPerm4Perm2(Obj f, Obj p)
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM4);
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM2);
 
-    UInt    deg, rank, degconj, i, j, k, codeg;
-    UInt4 * ptf, *ptconj, dep;
+    UInt    deg, dep, rank, degconj, i, j, k, codeg;
+    UInt4 * ptf, *ptconj;
     UInt2 * ptp;
     Obj     conj, dom;
 
@@ -3776,8 +3782,8 @@ Obj PowPPerm4Perm4(Obj f, Obj p)
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM4);
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM4);
 
-    UInt   deg, rank, degconj, i, j, k, codeg;
-    UInt4 *ptf, *ptp, *ptconj, dep;
+    UInt   deg, dep, rank, degconj, i, j, k, codeg;
+    UInt4 *ptf, *ptp, *ptconj;
     Obj    conj, dom;
 
     deg = DEG_PPERM4(f);
@@ -5398,8 +5404,8 @@ Obj LQuoPerm2PPerm2(Obj p, Obj f)
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM2);
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM2);
 
-    UInt2 *ptp, *ptf, *ptlquo, dep;
-    UInt   def, i, j, del, len;
+    UInt2 *ptp, *ptf, *ptlquo;
+    UInt   def, dep, i, j, del, len;
     Obj    dom, lquo;
 
     def = DEG_PPERM2(f);
@@ -5482,9 +5488,9 @@ Obj LQuoPerm2PPerm4(Obj p, Obj f)
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM2);
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM4);
 
-    UInt2 *ptp, dep;
+    UInt2 *ptp;
     UInt4 *ptf, *ptlquo;
-    UInt   def, i, j, del, len;
+    UInt   def, dep, i, j, del, len;
     Obj    dom, lquo;
 
     def = DEG_PPERM4(f);
@@ -5651,8 +5657,8 @@ Obj LQuoPerm4PPerm4(Obj p, Obj f)
     GAP_ASSERT(TNUM_OBJ(p) == T_PERM4);
     GAP_ASSERT(TNUM_OBJ(f) == T_PPERM4);
 
-    UInt4 *ptp, *ptf, *ptlquo, dep;
-    UInt   def, i, j, del, len;
+    UInt4 *ptp, *ptf, *ptlquo;
+    UInt   def, dep, i, j, del, len;
     Obj    dom, lquo;
 
     def = DEG_PPERM4(f);

diff --git a/tst/testinstall/pperm.tst b/tst/testinstall/pperm.tst
@@ -1,4 +1,4 @@
-#@local display,e,f,g,h,i,notationpp,notationt,p,x,PPerm4,Perm4,im,coll
+#@local display,e,f,g,h,i,notationpp,notationt,p,x,PPerm4,Perm4,im,coll,p1,p2
 ##
 ## takes around 4 seconds to run
 
@@ -1025,7 +1025,8 @@ gap> AsPermutation(f);
 # PermLeftQuoPartialPerm
 gap> f := PartialPerm([1, 100], [100, 2]);;
 gap> p := (2, 100);;
-gap> g:=f*p;;
+gap> g:=f*p;
+[1,2](100)
 gap> PermLeftQuoPartialPerm(f, g)=p;
 true
 gap> h := PartialPerm([200, 300, 400, 1900, 10 ^ 6], 
@@ -1226,7 +1227,8 @@ true
 gap> f:=PartialPerm( [ 1, 2, 3, 6, 10 ], [ 2, 7, 8, 10, 6 ] );;
 > DomainOfPartialPerm(f);;
 gap> p:=(7, 100);;
-gap> g:=f*p;;
+gap> g:=f*p;
+[1,2,100][3,8](6,10)
 gap> DomainOfPartialPerm(g)=DomainOfPartialPerm(f);
 true
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
@@ -1241,14 +1243,16 @@ gap> f * p;
 # ProdPPerm2Perm2, Case 2 of 6: codeg(f)<=deg(p), domain not known
 gap> f:=PartialPerm( [ 1, 2, 3, 6, 10 ], [ 2, 7, 8, 10, 6 ] );;
 gap> p:=(7, 100);;
-gap> g:=f*p;;
+gap> g:=f*p;
+[1,2,100][3,8](6,10)
 gap> DomainOfPartialPerm(g)=DomainOfPartialPerm(f);
 true
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
 true
 gap> f:=PartialPerm( [ 1, 2, 3, 6, 10 ], [ 2, 7, 8, 10, 6 ] );;
 gap> p:=(7, 100);;
-gap> g:=f*p;;
+gap> g:=f*p;
+[1,2,100][3,8](6,10)
 gap> DomainOfPartialPerm(g)=DomainOfPartialPerm(f);
 true
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
@@ -1259,7 +1263,8 @@ true
 # ProdPPerm2Perm2, Case 3 of 6: codeg(f)>deg(p), domain known
 gap> f:=PartialPerm([1, 100], [100, 2]);; DomainOfPartialPerm(f);;
 gap> p:=(7, 10);;
-gap> g:=f*p;;
+gap> g:=f*p;
+[1,100,2]
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
 true
 gap> DomainOfPartialPerm(g)=DomainOfPartialPerm(f);
@@ -1268,7 +1273,8 @@ gap> CodegreeOfPartialPerm(g)=Maximum(ImageSetOfPartialPerm(g));
 true
 gap> f:=PartialPerm([1, 65535], [65535, 2]);;
 gap> p:=(17, 10000);;
-gap> g:=f*p;;
+gap> g:=f*p;
+[1,65535,2]
 gap> DomainOfPartialPerm(g)=DomainOfPartialPerm(f);
 true
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
@@ -1278,17 +1284,19 @@ true
 
 # ProdPPerm2Perm2, Case 4 of 6: codeg(f)>deg(p), domain not known
 gap> f:=PartialPerm([1, 100], [100, 2]);;
-gap> p:=(7, 10);;   
-gap> g:=f*p;;                       
+gap> p:=(7, 10);;
+gap> g:=f*p;
+[1,100,2]
 gap> DomainOfPartialPerm(g)=DomainOfPartialPerm(f);
 true
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
 true
 gap> CodegreeOfPartialPerm(g)=Maximum(ImageSetOfPartialPerm(g));
 true
 gap> f:=PartialPerm([1, 10000], [10000, 2]);;
-gap> p:=(13, 1000);; 
-gap> g:=f*p;;
+gap> p:=(13, 1000);;
+gap> g:=f*p;
+[1,10000,2]
 gap> DomainOfPartialPerm(g)=DomainOfPartialPerm(f);
 true
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
@@ -1299,7 +1307,8 @@ true
 # ProdPPerm2Perm2, Case 5 of 6: deg(p)=65536, domain not known
 gap> p:=(1,65536);;
 gap> f:=PartialPerm([1, 10000], [10000, 2]);;
-gap> g:=f*p;;
+gap> g:=f*p;
+[1,10000,2]
 gap> DomainOfPartialPerm(g)=DomainOfPartialPerm(f);
 true
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
@@ -1310,7 +1319,8 @@ true
 # ProdPPerm2Perm2, Case 6 of 6: deg(p)=65536, domain known
 gap> f:=PartialPerm([1, 10000], [10000, 2]);; DomainOfPartialPerm(f);;
 gap> p:=(1,65536);;
-gap> g:=f*p;;
+gap> g:=f*p;
+[1,10000,2]
 gap> DomainOfPartialPerm(g)=DomainOfPartialPerm(f);
 true
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
@@ -1321,7 +1331,8 @@ true
 # ProdPPerm2Perm4, Case 1 of 2: domain known
 gap> f:=PartialPerm( [ 1, 2, 3, 6, 10 ], [ 2, 7, 8, 10, 6 ] );; DomainOfPartialPerm(f);;
 gap> p:=(1,100000);;
-gap> g:=f*p;;
+gap> g:=f*p;
+[1,2,7][3,8](6,10)
 gap> DomainOfPartialPerm(g)=DomainOfPartialPerm(f);
 true
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
@@ -1332,7 +1343,8 @@ true
 # ProdPPerm2Perm4, Case 2 of 2: domain not known
 gap> f:=PartialPerm([1, 1000], [1000, 2]);;
 gap> p:=(1,100000);;
-gap> g:=f*p;;
+gap> g:=f*p;
+[1,1000,2]
 gap> OnTuples(ImageListOfPartialPerm(f), p)=ImageListOfPartialPerm(g);
 true
 gap> CodegreeOfPartialPerm(g)=Maximum(ImageSetOfPartialPerm(g));
@@ -2962,6 +2974,72 @@ gap> f^g;
 gap> g^-1*f*g;
 [4,6][10,1][17,13](19)
 
+#
+# a bunch of tests involving T_PERM2 permutations of degree 65536
+#
+gap> f:=PartialPerm([0,0,1,5]);
+[3,1][4,5]
+gap> g:=PartialPermNC(Concatenation(List([1..65535], x-> 0), [1,100001]));
+[65536,1][65537,100001]
+gap> p1 := (1,65536);
+(1,65536)
+gap> p2 := (3,65536);
+(3,65536)
+
+# products
+gap> f*p1;
+[3,65536][4,5]
+gap> f*p2;
+[3,1][4,5]
+gap> p1*f;
+[3,1][4,5]
+gap> p2*f;
+[4,5][65536,1]
+gap> g*p1;
+[65537,100001](65536)
+gap> g*p2;
+[65536,1][65537,100001]
+gap> p1*g;
+[65537,100001](1)
+gap> p2*g;
+[3,1][65537,100001]
+
+# quotients
+gap> f/p1 = f*p1;
+true
+gap> f/p2 = f*p2;
+true
+gap> g/p1 = g*p1;
+true
+gap> g/p2 = g*p2;
+true
+gap> LQUO(p1,f) = p1*f;
+true
+gap> LQUO(p2,f) = p2*f;
+true
+gap> LQUO(p1,g) = p1*g;
+true
+gap> LQUO(p2,g) = p2*g;
+true
+
+# conjugation: f^p = p^-1 * f * p
+gap> f^p1;
+[3,65536][4,5]
+gap> f^p2;
+[4,5][65536,1]
+gap> g^p1;
+[1,65536][65537,100001]
+gap> g^p2;
+[3,1][65537,100001]
+gap> ListX([p1,p2],[f,g], {x,y} -> (x*y)*x = y^x);
+[ true, true, true, true ]
+gap> ListX([p1,p2],[f,g], {x,y} -> x*(y*x) = y^x);
+[ true, true, true, true ]
+
+#
+#
+#
+
 # from Semigroups...
 gap> f:=PartialPermNC([0,1,0,20]);
 [2,1][4,20]