-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better define the result of MappingPermListList #1432
Better define the result of MappingPermListList #1432
Conversation
@james-d-mitchell might have an opinion on this, as he originally merged the code from orb. |
Codecov Report
@@ 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/permutat.g
Outdated
## 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>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be simpler if it was "fixes any point which is not in src or dst"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
lib/permutat.g
Outdated
## 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>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If there is no such permutation"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/permutat.c
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -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]; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you check that src
and dst
are lists here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LEN_LIST
will do this -- it throws an Error
is passed a non-list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/permutat.c
Outdated
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous comment.
1ae7d99
to
9ad42c9
Compare
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.
9ad42c9
to
711383c
Compare
I think all of @james-d-mitchell 's changes are now made. |
This PR combines two fixes:
Fail
).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 forMappingPermListList
was totally rewritten.