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

NC versions of PreImages, PreImagesSet, PreImagesElm and PreImagesRepresentative #5073

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
4 changes: 2 additions & 2 deletions doc/ref/grpfp.xml
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ gap> hom:=IsomorphismFpGroup(u);
[ <[ [ 1, 1 ] ]|f2^-1*f1^-1> ] -> [ F1 ]
gap> new:=Range(hom);
<fp group on the generators [ F1 ]>
gap> List(GeneratorsOfGroup(new),i->PreImagesRepresentative(hom,i));
gap> List(GeneratorsOfGroup(new),i->PreImagesRepresentativeNC(hom,i));
[ <[ [ 1, 1 ] ]|f2^-1*f1^-1> ]
]]></Example>
<P/>
Expand All @@ -484,7 +484,7 @@ behave like ordinary words and no extra treatment should be necessary.
<Example><![CDATA[
gap> r:=Range(hom).1^10;
F1^10
gap> p:=PreImagesRepresentative(hom,r);
gap> p:=PreImagesRepresentativeNC(hom,r);
<[ [ 1, 10 ] ]|(f2^-1*f1^-1)^10>
]]></Example>

Expand Down
2 changes: 1 addition & 1 deletion doc/ref/mapping.xml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<!-- %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% -->
<!-- %% -->
<!-- %A mapping.xml GAP documentation Thomas Breuer -->
<!-- %A mapping.xml GAP documentation Thomas Breuer -->
<!-- %% -->
<!-- %% -->
<!-- %Y (C) 1998 School Math and Comp. Sci., University of St Andrews, Scotland -->
Expand Down
2 changes: 1 addition & 1 deletion doc/ref/rws.xml
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ gap> red:=ReducedForm(k,UnderlyingElement(melm));
f1^-1^3*f2^-1*f1^2
gap> melm:=ElementOfFpMonoid(FamilyObj(One(mon)),red);
f1^-1^3*f2^-1*f1^2
gap> gpelm:=PreImagesRepresentative(mhom,melm);
gap> gpelm:=PreImagesRepresentativeNC(mhom,melm);
f1^-3*f2^-1*f1^2
gap> r=gpelm;
true
Expand Down
41 changes: 21 additions & 20 deletions doc/tut/group.xml
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
<!-- %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% -->
<!-- %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% -->
<!-- %% -->
<!-- %W group.tex GAP documentation Thomas Breuer -->
<!-- %W & Frank Celler -->
<!-- %W & Martin Schönert -->
<!-- %W & Heiko Theißen -->
<!-- %W group.tex GAP documentation Thomas Breuer -->
<!-- %W & Frank Celler -->
<!-- %W & Martin Schönert -->
<!-- %W & Heiko Theißen -->
<!-- %% -->
<!-- %% -->
<!-- %Y Copyright 1997, Lehrstuhl D für Mathematik, RWTH Aachen, Germany -->
<!-- %Y Copyright 1997, Lehrstuhl D für Mathematik, RWTH Aachen, Germany -->
<!-- %% -->
<!-- %% This file contains a tutorial introduction to groups. -->
<!-- %% -->
Expand Down Expand Up @@ -168,10 +168,9 @@ action domain of <C>norm</C>. (It only happens that both are subsets of the
natural numbers.) We can now form images and preimages under the natural
homomorphism. The set of preimages of an element under <C>hom</C> is a coset
modulo <C>elab</C>.
We use the function <Ref Func="PreImages" BookName="ref"/> here because
<C>hom</C> is not
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can just be left as PreImages, in general in documentation we don't encourage people to use NC methods of functions. Of course at the moment it doesn't make any difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ChrisJefferson Is this rule formulated somewhere in the documentation?

(If we do not want to encourage using NC variants in the documentation then this pull request should not change examples from the manual to use PreImagesRepresentativeNC instead of PreImagesRepresentative.)

I would agree that the NC variants are mainly intended to be used inside functions where it is clear from the context that the tests are not necessary. On the other hand, one can argue that the same holds for an interactive GAP session, for example, if one wants to compute a preimage of an element that was just constructed as an element of the range of the mapping in question.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ThomasBreuer Looking around when defining function we often define the NC and non-NC version at the same time, but when we reference we often only reference one.

I do think if we are mentioning PreImagesNC we should say what the difference is -- I know someone could follow the links to PreImages and PreImagesNC, but it is a bit strange here to see both mentioned.

In general it is probably worth thinking about if we want to use PreImagesRepresentative or PreImagesRepresentativeNC in most examples. I am not sure in general how much slower PreImagesRepresentative is going to be, so which we should be encouraging as a default -- but I would hope PreImagesRepresentative, because it is safer for users who are beginners.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ChrisJefferson Thanks for your thoughts.

Currently I find one statement about NC variants in the Tutorial Manual, which talks about Sub<something> vs. Sub<something>NC, and one statement about Sub<struct>NC in the Reference Manual Section "Constructing Subdomains"
I think there should be a general statement about Func vs. FuncNC (in the Tutorial as well as in the Reference Manual), saying that the latter omits some argument checks and that the documentation of the individual functions will explain which checks are omitted.
Some example that illustrates the difference should be shown in the Tutorial: When one creates a subgroup of a group then calling SubgroupNC instead of Subgroup will avoid membership tests for the subgroup generators; in the case of a permutation group, this may mean that the computation of a stabilizer chain for the big group can be avoided, which would hopefully be not expensive; for a matrix group such as the Baby Monster, calling SubgroupNC will be the only reasonable way to create a subgroup, since asking for membership will be in general hopeless.

In the Reference Manual, Func and FuncNC should be documented in the same ManSection, and then cross-references need to mention only the non-NC variant.

Concerning the use of NC variants in manual examples, I see two possible strategies. Either we use only the non-NC variants, or we use the NC variants in all those cases where the context admits this (perhaps with a textual comment why the NC variant may be called in examples in the Tutorial and in those cases where this is not obvious).
I would expect that the differences in runtime are negligible in manual examples, and if this is not the case for some example then this example deserves a comment about this fact.

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 agree that it is not a good idea to mention NC variants in the tutorial manual, and will make the necessary changes. The more general comments are very interesting but are beyond the scope of this 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.

Removed NC references in doc/tut/group.xml and doc/tut/algvspc.xml. Had problems trying to push these - no idea why - so used --force. Hope that does not cause any problems.

a bijection, so an element of the range can have several preimages or
none at all.
We use the function <Ref Func="PreImages" BookName="ref"/> here
because <C>hom</C> is not a bijection,
so an element of the range can have several preimages or none at all.
<P/>
<Example><![CDATA[
gap> ker:= Kernel( hom );
Expand Down Expand Up @@ -942,18 +941,20 @@ gap> PreImage( hom, TrivialSubgroup(s3) ); # the kernel
Group([ (1,4)(2,3), (1,3)(2,4) ])
]]></Example>
<P/>
This homomorphism from <M>S_4</M> onto <M>S_3</M> is well known from elementary
group theory. Images of elements and subgroups under <C>hom</C> can be
This homomorphism from <M>S_4</M> onto <M>S_3</M> is well known from
elementary group theory.
Images of elements and subgroups under <C>hom</C> can be
calculated with the function <Ref Func="Image" BookName="ref"/>.
But since the mapping <C>hom</C> is not bijective, we cannot use the
function <Ref Func="PreImage" BookName="ref"/> for preimages of
elements (they can have several preimages). Instead, we have to use
<Ref Func="PreImagesRepresentative" BookName="ref"/>,
which returns one preimage if at least one
exists (and would return <K>fail</K> if none exists, which cannot occur for
But since the mapping <C>hom</C> is not bijective, we cannot use the
function <Ref Func="PreImage" BookName="ref"/> for preimages of elements
(they can have several preimages).
Instead, we have to use
<Ref Func="PreImagesRepresentative" BookName="ref"/>
which returns one preimage if at least one exists
(and would return <K>fail</K> if none exists, which cannot occur for
our surjective <C>hom</C>).
On the other hand, we can use <Ref Func="PreImage" BookName="ref"/> for the
preimage of a set (which always exists, even if it is empty).
On the other hand, we can use <Ref Func="PreImage" BookName="ref"/> for the
preimage of a set (which always exists, even if it is empty).
<P/>
Suppose we mistype the input when trying to construct a homomorphism as below.
<P/>
Expand Down
2 changes: 1 addition & 1 deletion lib/algebra.gi
Original file line number Diff line number Diff line change
Expand Up @@ -3672,7 +3672,7 @@ InstallMethod( CentralIdempotentsOfAlgebra,

until k>Length(ideals);

id:= List( ids, e -> PreImagesRepresentative( hom, e ) );
id:= List( ids, e -> PreImagesRepresentativeNC( hom, e ) );

# Now we lift the idempotents to the big algebra `A'. The
# first idempotent is lifted as follows:
Expand Down
2 changes: 1 addition & 1 deletion lib/algfp.gi
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@
if hom = fail then
TryNextMethod();
fi;
return PreImagesRepresentative( hom, r );
return PreImagesRepresentativeNC( hom, r );

Check warning on line 708 in lib/algfp.gi

View check run for this annotation

Codecov / codecov/patch

lib/algfp.gi#L708

Added line #L708 was not covered by tests
end ) );


Expand Down
20 changes: 10 additions & 10 deletions lib/alghom.gi
Original file line number Diff line number Diff line change
Expand Up @@ -577,26 +577,26 @@

#############################################################################
##
#M PreImagesRepresentative( <map>, <elm> ) . . . . . . for algebra g.m.b.i.
#M PreImagesRepresentativeNC( <map>, <elm> ) . . . . . for algebra g.m.b.i.
##
InstallMethod( PreImagesRepresentative,
InstallMethod( PreImagesRepresentativeNC,
"for algebra g.m.b.i., and element",
FamRangeEqFamElm,
[ IsGeneralMapping and IsAlgebraGeneralMappingByImagesDefaultRep,
IsObject ],
function( map, elm )
return PreImagesRepresentative( AsLeftModuleGeneralMappingByImages(map),
elm );
return PreImagesRepresentativeNC(
AsLeftModuleGeneralMappingByImages(map), elm );

Check warning on line 589 in lib/alghom.gi

View check run for this annotation

Codecov / codecov/patch

lib/alghom.gi#L588-L589

Added lines #L588 - L589 were not covered by tests
end );

InstallMethod( PreImagesRepresentative,
InstallMethod( PreImagesRepresentativeNC,
"for algebra g.m.b.i. knowing inverse, and element",
FamRangeEqFamElm,
[ IsGeneralMapping and IsAlgebraGeneralMappingByImagesDefaultRep
and HasInverseGeneralMapping,
IsObject ],
function( map, elm )
return ImagesRepresentative( InverseGeneralMapping(map), elm );
return ImagesRepresentative( InverseGeneralMapping(map), elm );

Check warning on line 599 in lib/alghom.gi

View check run for this annotation

Codecov / codecov/patch

lib/alghom.gi#L599

Added line #L599 was not covered by tests
end );


Expand Down Expand Up @@ -915,7 +915,7 @@

#############################################################################
##
#M PreImagesRepresentative( <ophom>, <mat> )
#M PreImagesRepresentativeOperationAlgebraHomomorphism( <ophom>, <mat> )
##
BindGlobal( "PreImagesRepresentativeOperationAlgebraHomomorphism", function( ophom, mat )
if not IsBound( ophom!.basisImage ) then
Expand All @@ -928,7 +928,7 @@
return mat;
end );

InstallMethod( PreImagesRepresentative,
InstallMethod( PreImagesRepresentativeNC,
"for an operation algebra homomorphism, and an element",
FamRangeEqFamElm,
[ IsOperationAlgebraHomomorphismDefaultRep, IsMatrix ],
Expand Down Expand Up @@ -1090,9 +1090,9 @@

#############################################################################
##
#M PreImagesRepresentative( <ophom>, <mat> )
#M PreImagesRepresentativeNC( <ophom>, <mat> )
##
InstallMethod( PreImagesRepresentative,
InstallMethod( PreImagesRepresentativeNC,
"for an alg. hom. from f. p. algebra, and an element",
FamRangeEqFamElm,
[ IsAlgebraHomomorphismFromFpRep, IsMatrix ],
Expand Down
26 changes: 13 additions & 13 deletions lib/alglie.gi
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
# under the natural homomorphism.
Add( S, C );
hom:= NaturalHomomorphismByIdeal( L, C );
C:= PreImages( hom, LieCentre( Range( hom ) ) );
C:= PreImagesNC( hom, LieCentre( Range( hom ) ) );
#T we would like to get ideals!
#T is it possible to teach the hom. that the preimage of an ideal is an ideal?

Expand Down Expand Up @@ -1707,7 +1707,7 @@
quo:= ImagesSource( hom );
r1:= LieSolvableRadical( quo );
B:= BasisVectors( Basis( r1 ) );
B:= List( B, x -> PreImagesRepresentative( hom, x ) );
B:= List( B, x -> PreImagesRepresentativeNC( hom, x ) );

Check warning on line 1710 in lib/alglie.gi

View check run for this annotation

Codecov / codecov/patch

lib/alglie.gi#L1710

Added line #L1710 was not covered by tests
Append( B, BasisVectors( Basis( n ) ) );

fi;
Expand Down Expand Up @@ -2100,7 +2100,7 @@
SetRadicalOfAlgebra( Q, Subalgebra( Q, [ Zero( Q ) ] ) );

id:= List( CentralIdempotentsOfAlgebra( Q ),
x->PreImagesRepresentative(hom,x));
x->PreImagesRepresentativeNC(hom,x));

Check warning on line 2103 in lib/alglie.gi

View check run for this annotation

Codecov / codecov/patch

lib/alglie.gi#L2103

Added line #L2103 was not covered by tests

# Now we lift the idempotents to the big algebra `A'. The
# first idempotent is lifted as follows:
Expand Down Expand Up @@ -4029,9 +4029,9 @@

###########################################################################
##
#M PreImagesRepresentative( f, x )
#M PreImagesRepresentativeNC( f, x )
##
InstallMethod( PreImagesRepresentative,
InstallMethod( PreImagesRepresentativeNC,
"for Fp to SCA mapping, and element",
FamRangeEqFamElm,
[ IsFptoSCAMorphism, IsSCAlgebraObj ], 0,
Expand Down Expand Up @@ -5643,8 +5643,8 @@
T:= EmptySCTable( dim , Zero(F) , "antisymmetric" );
pimgs := [];
for i in [1..dim] do
a:= PreImagesRepresentative( Homs[pos[i]] ,
PreImagesRepresentative( hom_pcg[pos[i]], gens[i] ) );
a:= PreImagesRepresentativeNC( Homs[pos[i]] ,
PreImagesRepresentativeNC( hom_pcg[pos[i]], gens[i] ) );

Check warning on line 5647 in lib/alglie.gi

View check run for this annotation

Codecov / codecov/patch

lib/alglie.gi#L5646-L5647

Added lines #L5646 - L5647 were not covered by tests

# calculate the p-th power image of `a':

Expand All @@ -5661,8 +5661,8 @@
# Calculate the commutator [a,b], and map the result into
# the correct homogeneous component.

b:= PreImagesRepresentative( Homs[pos[j]],
PreImagesRepresentative( hom_pcg[pos[j]], gens[j] ));
b:= PreImagesRepresentativeNC( Homs[pos[j]],
PreImagesRepresentativeNC( hom_pcg[pos[j]], gens[j] ));

Check warning on line 5665 in lib/alglie.gi

View check run for this annotation

Codecov / codecov/patch

lib/alglie.gi#L5664-L5665

Added lines #L5664 - L5665 were not covered by tests
c:= Image( hom_pcg[pos[i] + pos[j]],
Image(Homs[pos[i] + pos[j]], a^-1*b^-1*a*b) );
e:= ExtRepOfObj(c);
Expand Down Expand Up @@ -5839,8 +5839,8 @@
T:= EmptySCTable( dim , Zero(F) , "antisymmetric" );
pimgs := [];
for i in [1..dim] do
a:= PreImagesRepresentative( Homs[pos[i]] ,
PreImagesRepresentative( hom_pcg[pos[i]], gens[i] ) );
a:= PreImagesRepresentativeNC( Homs[pos[i]] ,
PreImagesRepresentativeNC( hom_pcg[pos[i]], gens[i] ) );

Check warning on line 5843 in lib/alglie.gi

View check run for this annotation

Codecov / codecov/patch

lib/alglie.gi#L5842-L5843

Added lines #L5842 - L5843 were not covered by tests


# calculate the p-th power image of `a':
Expand All @@ -5858,8 +5858,8 @@
# Calculate the commutator [a,b], and map the result into
# the correct homogeneous component.

b:= PreImagesRepresentative( Homs[pos[j]],
PreImagesRepresentative( hom_pcg[pos[j]], gens[j] ));
b:= PreImagesRepresentativeNC( Homs[pos[j]],
PreImagesRepresentativeNC( hom_pcg[pos[j]], gens[j] ));

Check warning on line 5862 in lib/alglie.gi

View check run for this annotation

Codecov / codecov/patch

lib/alglie.gi#L5861-L5862

Added lines #L5861 - L5862 were not covered by tests
c:= Image( hom_pcg[pos[i] + pos[j]],
Image(Homs[pos[i] + pos[j]], a^-1*b^-1*a*b) );
e:= ExtRepOfObj(c);
Expand Down
12 changes: 7 additions & 5 deletions lib/algrep.gd
Original file line number Diff line number Diff line change
Expand Up @@ -697,10 +697,12 @@ DeclareOperation( "ModuleByRestriction", [ IsAlgebraModule, IsAlgebra ] );
## <Oper Name="NaturalHomomorphismBySubAlgebraModule" Arg='V, W'/>
##
## <Description>
## Here <A>V</A> must be a sub-algebra module of <A>V</A>. This function returns
## the projection from <A>V</A> onto <C><A>V</A>/<A>W</A></C>. It is a linear map, that is
## also a module homomorphism. As usual images can be formed with
## <C>Image( f, v )</C> and pre-images with <C>PreImagesRepresentative( f, u )</C>.
## Here <A>V</A> must be a sub-algebra module of <A>V</A>.
## This function returns
## the projection from <A>V</A> onto <C><A>V</A>/<A>W</A></C>.
## It is a linear map, that is also a module homomorphism.
## As usual images can be formed with <C>Image( f, v )</C>
## and pre-images with <C>PreImagesRepresentativeNC( f, u )</C>.
## <P/>
## The quotient module can also be formed
## by entering <C><A>V</A>/<A>W</A></C>.
Expand All @@ -726,7 +728,7 @@ DeclareOperation( "ModuleByRestriction", [ IsAlgebraModule, IsAlgebra ] );
## 18 over Rationals>>
## gap> v:= Basis( quo )[1];
## [ 1, 0, 0, 0, 0, 0, 0, 0, 0 ]
## gap> PreImagesRepresentative( f, v );
## gap> PreImagesRepresentativeNC( f, v );
## v.4
## gap> Basis( C )[4]^v;
## [ 1, 0, 0, 0, 0, 0, 0, 0, 0 ]
Expand Down
8 changes: 4 additions & 4 deletions lib/algrep.gi
Original file line number Diff line number Diff line change
Expand Up @@ -1212,24 +1212,24 @@
if IsLeftAlgebraModuleElementCollection( V ) then
if IsRightAlgebraModuleElementCollection( V ) then
left_op:= function( x, v )
return ImagesRepresentative( f, x^PreImagesRepresentative( f, v ) );
return ImagesRepresentative( f, x^PreImagesRepresentativeNC( f, v ) );

Check warning on line 1215 in lib/algrep.gi

View check run for this annotation

Codecov / codecov/patch

lib/algrep.gi#L1215

Added line #L1215 was not covered by tests
end;
right_op:= function( v, x )
return ImagesRepresentative( f, PreImagesRepresentative( f, v )^x );
return ImagesRepresentative( f, PreImagesRepresentativeNC( f, v )^x );

Check warning on line 1218 in lib/algrep.gi

View check run for this annotation

Codecov / codecov/patch

lib/algrep.gi#L1218

Added line #L1218 was not covered by tests
end;
qmod:= BiAlgebraModule( LeftActingAlgebra( V ),
RightActingAlgebra( V ),
left_op, right_op, quot );
else
left_op:= function( x, v )
return ImagesRepresentative( f, x^PreImagesRepresentative( f, v ) );
return ImagesRepresentative( f, x^PreImagesRepresentativeNC( f, v ) );

Check warning on line 1225 in lib/algrep.gi

View check run for this annotation

Codecov / codecov/patch

lib/algrep.gi#L1225

Added line #L1225 was not covered by tests
end;
qmod:= LeftAlgebraModule( LeftActingAlgebra( V ),
left_op, quot);
fi;
else
right_op:= function( v, x )
return ImagesRepresentative( f, PreImagesRepresentative( f, v )^x );
return ImagesRepresentative( f, PreImagesRepresentativeNC( f, v )^x );

Check warning on line 1232 in lib/algrep.gi

View check run for this annotation

Codecov / codecov/patch

lib/algrep.gi#L1232

Added line #L1232 was not covered by tests
end;
qmod:= RightAlgebraModule( RightActingAlgebra( V ),
right_op, quot );
Expand Down
Loading