Better define the result of MappingPermListList #1432

ChrisJefferson · 2017-06-19T13:06:12Z

This PR combines two fixes:

When MappingPermListList was translated to C, a number of bugs were introduced:

Wrong answer when there is no permutation mapping one list to the other (sometimes returns a perm instead of Fail).
GAP seg-faults when input lists contain negative integers or non-integers.

MappingPermListList is used by RepresentativeAction on alternating and symmetric groups. Here, RepresenatativeAction assumes that values not in either list will not be moved. This is not true.

I have adjusted MappingPermListList to not move points in either input list. This produces a different permutation, but does not seem to slow things down, and is also acceptable according to the documentation.

The bug in Part (2) (RepresentativeAction returns incorrect answers) also exists in stable, but will need a totally different patch, as the code for MappingPermListList was totally rewritten.

ChrisJefferson · 2017-06-19T13:11:21Z

@james-d-mitchell might have an opinion on this, as he originally merged the code from orb.

codecov · 2017-06-19T13:38:48Z

Codecov Report

Merging #1432 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1432      +/-   ##
==========================================
+ Coverage   63.45%   63.46%   +<.01%     
==========================================
  Files        1011     1011              
  Lines      339023   339058      +35     
  Branches    13741    13755      +14     
==========================================
+ Hits       215143   215192      +49     
+ Misses     120845   120833      -12     
+ Partials     3035     3033       -2

Impacted Files	Coverage Δ
lib/permutat.g	`75.43% <ø> (ø)`	⬆️
src/permutat.c	`73.23% <100%> (+0.61%)`	⬆️
src/objset.c	`40.57% <0%> (-1.15%)`	⬇️
src/listfunc.c	`76.77% <0%> (-0.18%)`	⬇️
src/hpc/thread.c	`46.54% <0%> (ø)`	⬆️
hpcgap/src/stats.c	`73.16% <0%> (ø)`	⬆️
src/funcs.c	`69.94% <0%> (+0.14%)`	⬆️
hpcgap/lib/hpc/stdtasks.g	`38.87% <0%> (+0.25%)`	⬆️
lib/gpprmsya.gi	`63.56% <0%> (+0.37%)`	⬆️
... and 3 more

james-d-mitchell

Looks good, other than the minor things mentioned. I guess that this change may cause the Semigroups test to fail because of the change in the permutation this returns. I'd like to double check that it doesn't cause any other problems before this is merged, if that's ok.

james-d-mitchell · 2017-06-19T19:47:49Z

lib/permutat.g

-##  The permutation <M>\pi</M> fixes all points larger than the maximum of
-##  the entries in <A>src</A> and <A>dst</A>.
+##  The permutation <M>\pi</M> fixes all points which are not contained in
+##  either of <A>src</A> or <A>dst</A>.


This would be simpler if it was "fixes any point which is not in src or dst"

james-d-mitchell · 2017-06-19T19:48:16Z

lib/permutat.g

 ##  If there are several such permutations, it is not specified which of them
-##  <Ref Func="MappingPermListList"/> returns.
+##  <Ref Func="MappingPermListList"/> returns. If there are no such
+##  permutations, then <Ref Func="MappingPermListList"/> returns <K>fail</K>.


"If there is no such permutation"

james-d-mitchell · 2017-06-19T19:49:41Z

src/permutat.c

-        x = INT_INTOBJ(ELM_LIST(src,i));
+        obj = ELM_LIST(src, i);
+        if (!IS_POS_INTOBJ(obj)) {
+            ErrorMayQuit("Lists can only contain positive integers", 0L, 0L);


This error message is a bit too general, how about "The first argument must be a list of positive integers", arguably lists in GAP can contain things other than positive integers, just not in this function.

james-d-mitchell · 2017-06-19T19:50:05Z

src/permutat.c

@@ -4443,6 +4443,7 @@ Obj FuncMappingPermListList(Obj self, Obj src, Obj dst)
    Obj out;
    Obj tabdst, tabsrc;
    Int x;
+    Obj obj;
    Int mytabs[DEGREELIMITONSTACK+1];
    Int mytabd[DEGREELIMITONSTACK+1];



Shouldn't you check that src and dst are lists here too?

LEN_LIST will do this -- it throws an Error is passed a non-list.

Right, but not a very meaningful error, in this context, it'd be easier to find the source of the problem if there is an explicit error here.

james-d-mitchell · 2017-06-19T19:50:19Z

src/permutat.c

-        x = INT_INTOBJ(ELM_LIST(dst,i));
+        obj = ELM_LIST(dst, i);
+        if (!IS_POS_INTOBJ(obj)) {
+            ErrorMayQuit("Lists can only contain positive integers", 0L, 0L);


Same as previous comment.

This fixes some bugs introduced in moving MappingPermListList into the kernel: * Crash when given negative integers. * Crash when given non-integer types. * Sometimes (incorrectly) returns a permutation when given two lists where no perm exists mapping one to the other. This function also alters the permutation generated by MappingPermListList to only move points in one of the input lists. This fixes a bug in RepresentativeAction, which assumed this was the case.

ChrisJefferson · 2017-06-20T20:26:01Z

I think all of @james-d-mitchell 's changes are now made.

This was referenced Jun 19, 2017

MappingPermListList buggy compared to 4.8 #1430

Closed

RepresentativeAction fails on Symmetric and Alternating Groups #1431

Closed

james-d-mitchell requested changes Jun 19, 2017

View reviewed changes

ChrisJefferson force-pushed the mappinglistlist branch 2 times, most recently from 1ae7d99 to 9ad42c9 Compare June 19, 2017 22:56

ChrisJefferson force-pushed the mappinglistlist branch from 9ad42c9 to 711383c Compare June 20, 2017 16:57

james-d-mitchell approved these changes Jun 20, 2017

View reviewed changes

james-d-mitchell merged commit c31c55c into gap-system:master Jun 20, 2017

fingolfin mentioned this pull request Jul 26, 2017

Fix RepresentativeAction for symmetric and alternating groups (for master branch) #1537

Closed

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

olexandr-konovalov added this to the GAP 4.9.0 milestone Jan 20, 2018

olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better define the result of MappingPermListList #1432

Better define the result of MappingPermListList #1432

ChrisJefferson commented Jun 19, 2017 •

edited

Loading

ChrisJefferson commented Jun 19, 2017

codecov bot commented Jun 19, 2017 •

edited

Loading

james-d-mitchell left a comment

james-d-mitchell Jun 19, 2017

ChrisJefferson Jun 19, 2017

james-d-mitchell Jun 19, 2017

ChrisJefferson Jun 19, 2017

james-d-mitchell Jun 19, 2017

ChrisJefferson Jun 19, 2017

james-d-mitchell Jun 19, 2017

ChrisJefferson Jun 19, 2017

james-d-mitchell Jun 20, 2017

james-d-mitchell Jun 19, 2017

ChrisJefferson commented Jun 20, 2017

Better define the result of MappingPermListList #1432

Better define the result of MappingPermListList #1432

Conversation

ChrisJefferson commented Jun 19, 2017 • edited Loading

ChrisJefferson commented Jun 19, 2017

codecov bot commented Jun 19, 2017 • edited Loading

Codecov Report

james-d-mitchell left a 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

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

ChrisJefferson commented Jun 20, 2017

ChrisJefferson commented Jun 19, 2017 •

edited

Loading

codecov bot commented Jun 19, 2017 •

edited

Loading