Skip to content
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

problem with TestIdentityAction #3279

Closed
ThomasBreuer opened this issue Feb 11, 2019 · 2 comments · Fixed by #3281
Closed

problem with TestIdentityAction #3279

ThomasBreuer opened this issue Feb 11, 2019 · 2 comments · Fixed by #3281
Labels
kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them
Milestone

Comments

@ThomasBreuer
Copy link
Contributor

The following happens in the current master branch as well as in released versions
(at least 4.10.0, 4.9.1, 4.7.6).

gap> g:= GL(2,3);;
gap> SparseActionHomomorphism( g, [ [ Z(3), Z(3) ] ] );
Error, Action not well-defined. See the manual section
``Action on canonical representatives''. at /.../gapmaster/gap/lib/oprt.gd:441 called from
TestIdentityAction( acts, pnt, act 
 ) at /.../gapmaster/gap/lib/oprt.gd:840 called from
...

The function SparseActionHomomorphism is called in a generic NiceMonomorphism method for matrix groups, try for example Size( Group( GeneratorsOfGroup( GL( 2, Integers mod 6 ) ) ) ).
The arguments of the (undocumented) function TestIdentityAction are the list of group generators, the list of seed vectors, and the action function (here the default OnPoints),
and the idea behind TestIdentityAction is to check whether the One of the first generator fixes each seed vector w.r.t. the action function.
In the situations where SparseActionHomomorphism is called in GAP library code,
this does not run into an error because then the second argument is the (list of rows of) the identity element in the matrix group.

gap> g:= GL(2,3);;
gap> SparseActionHomomorphism( g, One( g ) );
<action epimorphism>

Note that in this case, TestIdentityAction tests in fact whether the One of the first generator conjugates the list of seed vectors to itself, in the sense that <pnt>^<one> is computed as <one> * <pnt> * <one>, which is fine only if <pnt> is square.
If one specifies the action function OnRight instead of OnPoints then the above example works.
(There is even a manual example that shows this situation.)

gap> g:= GL(2,3);;
gap> SparseActionHomomorphism( g, [ [ Z(3), Z(3) ] ], OnRight );
<action epimorphism>

I think that the check in TestIdentityAction must apply the identity element to each seed vector separately, since we cannot assume that the action function knows how to deal with a list of seed vectors.
Perhaps the cleanest solution would be to replace the TestIdentityAction call by an appropriate Assert call; note that TestIdentityAction seems to be not used in other places.
The only argument against this change is that currently the list of seed vectors gets replaced by the result of TestIdentityAction (why?), but on the other hand it seems to be allowed to skip TestIdentityAction via the option NoTestAction.

@ThomasBreuer ThomasBreuer added kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them backport-to-4.10 labels Feb 11, 2019
hulpke added a commit to hulpke/gap that referenced this issue Feb 12, 2019
but need to use their own creator function (as point seed is list of points and
thus need to apply `TestIdentityAction` differently.)

This fixes gap-system#3279
hulpke added a commit to hulpke/gap that referenced this issue Feb 12, 2019
but need to use their own creator function (as point seed is list of points and
thus need to apply `TestIdentityAction` differently.)

This fixes gap-system#3279
hulpke added a commit to hulpke/gap that referenced this issue Feb 12, 2019
but need to use their own creator function (as point seed is list of points and
thus need to apply `TestIdentityAction` differently.)

This fixes gap-system#3279
@hulpke
Copy link
Contributor

hulpke commented Feb 12, 2019

As far as I can see the problem is with declaring SparseActionHomomorphism through OrbitishFO (Orbit-style operations take one seed only).
This must have been a bug all the time, and just never showed up.
I have submitted a PR that splits this up (and then calls TestIdentityAction separately on each of the seeds).

As for calling TestIdentityAction in the first place, this is (prompted by bug reports) to catch issues, such as Orbit(G,[1,3,2],OnSets), in which the point does not satisfy implicit assumptions on the action.

ThomasBreuer pushed a commit that referenced this issue Feb 14, 2019
but need to use their own creator function (as point seed is list of points and
thus need to apply `TestIdentityAction` differently.)

This fixes #3279
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.1 milestone Feb 19, 2019
@olexandr-konovalov
Copy link
Member

I removed "backport-to-4.10" label from this issue - one can not really backport an issue. But one can backport a pull request, and I have added appropriate labels to #3281.

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Mar 21, 2019
ssiccha pushed a commit to ssiccha/gap that referenced this issue Mar 27, 2019
but need to use their own creator function (as point seed is list of points and
thus need to apply `TestIdentityAction` differently.)

This fixes gap-system#3279
fingolfin pushed a commit that referenced this issue Jun 13, 2019
but need to use their own creator function (as point seed is list of points and
thus need to apply `TestIdentityAction` differently.)

This fixes #3279
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants