Implement CycleFromList, and fix potential crash in CYCLE_TRANS_INT #2242

markusbaumeister · 2018-03-05T09:47:18Z

Add a convenient functionality to generate a cycle from a list of
positive integers.

fingolfin

I agree that something like this is useful to have in general.

I wonder if instead of just supporting creating cycles, it would be possible / useful to have something which takes a list-of-lists (representing disjoint cycles), and turns those into a permutation. (a simple implementation would just call your function here on each cycle, and then multiply the result together, though of course that would not detect overlaps; which might be considered either a bug or feature ;-)

codecov · 2018-03-05T10:36:53Z

Codecov Report

Merging #2242 into master will increase coverage by 0.45%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #2242      +/-   ##
==========================================
+ Coverage   69.88%   70.34%   +0.45%     
==========================================
  Files         481      481              
  Lines      252990   253198     +208     
==========================================
+ Hits       176813   178117    +1304     
+ Misses      76177    75081    -1096

Impacted Files	Coverage Δ
src/trans.c	`98.71% <100%> (ø)`	⬆️
lib/permutat.g	`79.54% <88.88%> (+1.47%)`	⬆️
lib/csetperm.gi	`87.65% <0%> (-3.71%)`	⬇️
lib/global.gi	`40.14% <0%> (-1.7%)`	⬇️
lib/ghompcgs.gi	`88.18% <0%> (-1.58%)`	⬇️
lib/ghom.gi	`72.83% <0%> (-0.82%)`	⬇️
lib/grppcint.gi	`98.64% <0%> (-0.68%)`	⬇️
lib/gpfpiso.gi	`60.17% <0%> (-0.39%)`	⬇️
lib/stbcrand.gi	`90.26% <0%> (-0.2%)`	⬇️
lib/gpprmsya.gi	`65.25% <0%> (-0.18%)`	⬇️
... and 29 more

ChrisJefferson · 2018-03-05T14:01:06Z

lib/permutat.g

+    fi;
+
+    set := Set(list);
+    if not IsDenseList(list) then


I don't think you need this check, as the next check will fail if the list is not sense (as the set will be smaller).

Also, maybe check Minimum is > 0

fingolfin · 2018-03-05T16:23:39Z

lib/permutat.g

+##  return the <M>n</M>-cycle <M>(a_1,a_2,...,a_n)</M>. For the empty list
+##  the trivial permutation <M>()</M> is returned.
+##  <P/>
+##  If the given <A>list</A> contains duplicates or holes, return <K>fail</K>.


And if there is a something that is not a positive integer in the list? Right now, probably various errors, depending on whether its a negative integer (gives proper error), a string or some such (will run into a method not found at some point), a rational (same, but likely in a different) spot, ...

fingolfin · 2018-03-05T16:26:04Z

lib/permutat.g

+    min := Minimum( set );
+    if min <= 0 then
+        Error("CycleFromList: List must contain only positive integers.");
+    fi;


Instead of this, perhaps do if not ForAll(list, IsPosInt) then Error(...); fi; even before computing set?

fingolfin · 2018-03-05T16:28:28Z

lib/permutat.g

+    od;
+    images[ list[Length(list)] ] := list[1];
+
+    return PermList(images);


While manual test cases are provided, and exercise the regular parts of the function, not all failure cases are tested, e.g. CycleFromList([0]), CycleFromList([1/2]), CycleFromList([true]) to name a few. Perhaps add some (though perhaps to a .tst file, not the manual example)?

I'll not block the PR over this, but it is needed to maximize the test coverage.

There is also no test for the empty list. All this can be seen on https://codecov.io/gh/gap-system/gap/pull/2242/diff?src=pr&el=tree#diff-bGliL3Blcm11dGF0Lmc= (link comes from the codecov report in the PR)

I don't know much about the test directory structure of GAP. Where exactly should I put the tests?

Somewhere into tst/testinstall/. In this case, I'd suggest to create tst/testinstall/opers/CyclesFromList.tst. Look at the other files in that directory to get an idea what to put in, in case you are not familiar with .tst files. And feel free to ask more questions.

fingolfin · 2018-03-07T13:15:10Z

I suspect the test failure is due to a bug in CYCLE_TRANS_INT, and your PR is just by luck triggering the bug. Try if this patch (added to a new commit helps)

diff --git a/src/trans.c b/src/trans.c
index deb807172..f0ea6c1ea 100644
--- a/src/trans.c
+++ b/src/trans.c
@@ -3102,6 +3102,7 @@ Obj FuncCYCLE_TRANS_INT(Obj self, Obj f, Obj pt)
         i = cpt;
         do {
             AssPlist(out, ++len, INTOBJ_INT(i + 1));
+            ptf2 = CONST_ADDR_TRANS2(f);
             i = ptf2[i];
         } while (i != cpt);
     }
@@ -3116,6 +3117,7 @@ Obj FuncCYCLE_TRANS_INT(Obj self, Obj f, Obj pt)
         i = cpt;
         do {
             AssPlist(out, ++len, INTOBJ_INT(i + 1));
+            ptf4 = CONST_ADDR_TRANS4(f);
             i = ptf4[i];
         } while (i != cpt);
     }

fingolfin · 2018-03-08T09:08:13Z

tst/testinstall/opers/CyclesFromList.tst

@@ -0,0 +1,33 @@
+gap> START_TEST("CyclesFromList")


This is missing a semicolon, hence the test fails.

You can test the .tst file locally using Test(), just do

Test("tst/testinstall/opers/CyclesFromList.tst");

(adjust the path depending on from where you run GAP)

Add a convenient functionality to generate a cycle from a list of positive integers.

The addition of CycleFromList uncovered a bug in CYCLE_TRANS_INT. This patch by Max Horn hopefully fixes that. His failure description (abbreviated) was: The problem is that `AssPlist` can trigger a GC. The change by chance happened to fill the heap in such a way that a GC happened at that spot

fingolfin reviewed Mar 5, 2018

View reviewed changes

ChrisJefferson reviewed Mar 5, 2018

View reviewed changes

markusbaumeister force-pushed the CycleList branch from 644d850 to 67f86c2 Compare March 5, 2018 15:04

fingolfin reviewed Mar 5, 2018

View reviewed changes

markusbaumeister force-pushed the CycleList branch from 67f86c2 to 1d005e0 Compare March 5, 2018 23:30

markusbaumeister force-pushed the CycleList branch from d09c355 to d3af460 Compare March 8, 2018 08:23

fingolfin reviewed Mar 8, 2018

View reviewed changes

markusbaumeister added 2 commits March 8, 2018 10:20

Implement CyclesFromList

3c75753

Add a convenient functionality to generate a cycle from a list of positive integers.

markusbaumeister force-pushed the CycleList branch from d3af460 to 9645f93 Compare March 8, 2018 09:20

fingolfin changed the title ~~Implement CyclesFromList~~ Implement CyclesFromList, and fix potential crash in CYCLE_TRANS_INT Mar 10, 2018

fingolfin approved these changes Mar 20, 2018

View reviewed changes

fingolfin added the kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements label Mar 20, 2018

fingolfin added this to the GAP 4.10.0 milestone Mar 20, 2018

fingolfin merged commit db5583e into gap-system:master Mar 20, 2018

olexandr-konovalov changed the title ~~Implement CyclesFromList, and fix potential crash in CYCLE_TRANS_INT~~ Implement CycleFromList, and fix potential crash in CYCLE_TRANS_INT Nov 1, 2018

fingolfin mentioned this pull request Jul 26, 2019

CycleFromList( [1,2,3,4] ) fails #3594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CycleFromList, and fix potential crash in CYCLE_TRANS_INT #2242

Implement CycleFromList, and fix potential crash in CYCLE_TRANS_INT #2242

markusbaumeister commented Mar 5, 2018

fingolfin left a comment

codecov bot commented Mar 5, 2018 •

edited

Loading

ChrisJefferson Mar 5, 2018

ChrisJefferson Mar 5, 2018

fingolfin Mar 5, 2018

fingolfin Mar 5, 2018

fingolfin Mar 5, 2018

fingolfin Mar 5, 2018

markusbaumeister Mar 5, 2018

fingolfin Mar 8, 2018

fingolfin commented Mar 7, 2018

fingolfin Mar 8, 2018

Implement CycleFromList, and fix potential crash in CYCLE_TRANS_INT #2242

Implement CycleFromList, and fix potential crash in CYCLE_TRANS_INT #2242

Conversation

markusbaumeister commented Mar 5, 2018

fingolfin left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 5, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Mar 7, 2018

Choose a reason for hiding this comment

codecov bot commented Mar 5, 2018 •

edited

Loading