Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/ref/permutat.xml
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ false
<#Include Label="PermList">
<#Include Label="MappingPermListList">
<#Include Label="RestrictedPerm">
<#Include Label="CycleFromList">

<ManSection>
<Attr Name="AsPermutation" Arg="f"/>
Expand Down
58 changes: 58 additions & 0 deletions lib/permutat.g
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,64 @@ BIND_GLOBAL( "ListPerm", function( arg )
end );


#############################################################################
##
#F CycleFromList( <list> ) . . . . . . . . . . . cycle defined from a list
##
## <#GAPDoc Label="CycleFromList">
## <ManSection>
## <Func Name="CycleFromList" Arg='list'/>
##
## <Description>
## For the given dense, duplicate-free list of positive integers
## <M>[a_1, a_2, ..., a_n]</M>
## 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>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, ...

## <P/>
## <Example><![CDATA[
## gap> CycleFromList( [1,2,3,4] );
## (1,2,3,4)
## gap> CycleFromList( [3,2,6,4,5] );
## (2,6,4,5,3)
## gap> CycleFromList( [2,3,2] );
## fail
## gap> CycleFromList( [1,,3] );
## fail
## ]]></Example>
## </Description>
## </ManSection>
## <#/GAPDoc>
##
BIND_GLOBAL( "CycleFromList", function( list )
local max, images, set, i;

# Trivial case
if Length(list) = 0 then
return ();
fi;

if ForAny( list, i -> not IsPosInt(i) ) then
Error("CycleFromList: List must only contain positive integers.");
fi;

set := Set(list);
if Length(set) <> Length(list) then
# we found duplicates (or list was not dense)
return fail;
fi;
max := Maximum( set );
images := [1..max];
for i in [1..Length(list)-1] do
images[ list[i] ] := list[i+1];
od;
images[ list[Length(list)] ] := list[1];

return PermList(images);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end );


#############################################################################
##
#O RestrictedPerm(<perm>,<list>) restriction of a perm. to an invariant set
Expand Down
2 changes: 2 additions & 0 deletions src/trans.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down
32 changes: 32 additions & 0 deletions tst/testinstall/opers/CyclesFromList.tst
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
gap> START_TEST("CyclesFromList");

#
gap> CycleFromList( [1..10] );
(1,2,3,4,5,6,7,8,9,10)
gap> CycleFromList( [1,5,4,8] );
(1,5,4,8)
gap> CycleFromList( [9,10,3,5]);
(3,5,9,10)
gap> CycleFromList( [1,3,,9] );
fail
gap> CycleFromList( [1,3,3,8] );
fail

# Errors
gap> CycleFromList( [7,2,0,3] );
Error, CycleFromList: List must only contain positive integers.
gap> CycleFromList( [9,3,-7,8,9] );
Error, CycleFromList: List must only contain positive integers.
gap> CycleFromList( [2,2,1/2,6] );
Error, CycleFromList: List must only contain positive integers.

#
gap> CycleFromList( [] );
()
gap> CycleFromList( [7] );
()
gap> CycleFromList( [0] );
Error, CycleFromList: List must only contain positive integers.
gap> CycleFromList( [true] );
Error, CycleFromList: List must only contain positive integers.
gap> STOP_TEST("CyclesFromList", 1);