Fix recursion depth trap and other improvements for quotients of fp groups #1884

markuspf · 2017-11-10T14:57:43Z

While addressing the immediate problem with #405, I stumbled across some more things that I added.

The immediate problem in #405 is that the quotient did not know it had GeneratorsOfSemigroup, so now the RecursionDepthTrap problem does not occur anymore (when taking the quotient of a group by a semigroup congruence). This was fixed by making the quotient aware of the presence of semigroup generators.

Using the code pasted in #405 now produces the following behaviour:

gap> f:=FreeGroup(2);;
gap> g := f / [[f.1^2, f.2^2]];
<fp group of size infinity on the generators [ f1, f2 ]>

This is because this PR installs a method to handle quotients of free groups by lists of equations, by delegating to quotients of free groups by relators, rewriting the equations [u,v] to the relators uv^{-1}.

Additionally this PR installs a method to take quotients of finitely presented groups by relators (and pairs of equations), as these were not previously installed.

Closes #405 .

fingolfin

This PR adds a new feature, but does not document it. Is that intentional?

fingolfin · 2017-11-10T15:42:56Z

lib/grpfp.gi

+    {F, rels} -> FactorFreeGroupByRelators(F, List(rels, r -> r[1] * r[2]^-1))); 
+
+InstallOtherMethod( \/,
+    "for free groups and a list of equations",


fingolfin · 2017-11-10T15:43:47Z

lib/grpfp.gi

+    IsElmsColls,
+    [ IsFreeGroup, IsCollection ],
+    0,
+    {F, rels} -> FactorFreeGroupByRelators(F, List(rels, r -> r[1] * r[2]^-1))); 


Perhaps write r -> r[1] / r[2] for brevity? Not that it's important.

fingolfin · 2017-11-10T15:44:54Z

It would be nice if @hulpke could review this, too.

We install methods to take quotients of finitely presented groups by lists of relations, and quotients of free and finitely presented groups by lists of equations (pairs of left hand side and right hand side).

codecov · 2017-11-10T15:56:05Z

Codecov Report

Merging #1884 into master will increase coverage by 7.55%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master    #1884      +/-   ##
==========================================
+ Coverage   56.56%   64.11%   +7.55%     
==========================================
  Files         898      917      +19     
  Lines      273508   282686    +9178     
  Branches    12764    12765       +1     
==========================================
+ Hits       154697   181244   +26547     
+ Misses     116068    98647   -17421     
- Partials     2743     2795      +52

Impacted Files	Coverage Δ
lib/semiquo.gi	`89.47% <100%> (+0.58%)`	⬆️
lib/grpfp.gi	`64.06% <50%> (+3.88%)`	⬆️
...ern/libatomic_ops/src/atomic_ops/sysdeps/gcc/x86.h	`100% <0%> (ø)`	⬆️
hpcgap/pkg/gapdoc/lib/GAPDoc2HTML.gi	`0% <0%> (ø)`
hpcgap/pkg/gapdoc/lib/Text.gi	`41.11% <0%> (ø)`
hpcgap/pkg/gapdoc/lib/BibTeX.gi	`0% <0%> (ø)`
hpcgap/pkg/gapdoc/lib/GAPDoc2Text.gi	`0.52% <0%> (ø)`
hpcgap/pkg/gapdoc/lib/ComposeXML.gi	`0% <0%> (ø)`
hpcgap/pkg/gapdoc/lib/PrintUtil.gi	`34.69% <0%> (ø)`
hpcgap/pkg/gapdoc/lib/XMLParser.gi	`0% <0%> (ø)`
... and 270 more

stevelinton · 2017-11-12T14:02:02Z

When quotienting a fp group by relators or equations are the relators or equations written over the FP group, or the underlying free group?

markuspf · 2017-11-13T11:25:03Z

@stevelinton good point. I only installed a method for / that calls FactorGroupFpGroupByRels, and that function expects elements of the fp group.

Both functionalities would make sense and we could install methods for both (or, I am overlooking something and we should not install methods for any?).

hulpke

The use fpgroup / words in free looks inconsistent to me. (This concerns the / operator. I'd be happy for a specific function if there is need for it.)

The more generic generalization seems to me to be
<group>/<elmlist> installed with IsCollsElms that would form <group>/N where N is the smallest normal subgroup containing <elmlist>. In the case of fp groups these are words, but it would also work for other groups (and redirect to FactorGroup) and remain consistent with the use for free groups.

markuspf · 2017-11-13T16:59:53Z

@hulpke as said above, I only installed a method that employs an already existing function, but I do like your suggestion (installing a method for <group> / <elmlist>) much better. I'll update the PR appropriately.

Can I just inquire what you refer to by "words" in this context: Is this strings in a generating set of a group in question (not the underlying free group for fp groups)?

markuspf · 2017-11-13T22:26:13Z

Just to clarify, I seem to have confused myself. With this PR the following happens

gap> F := FreeGroup(2);
<free group on the generators [ f1, f2 ]>
gap> G := F / [ F.1 * F.2 ];
<fp group of size infinity on the generators [ f1, f2 ]>
gap> H := G / [ G.1^2 ];
<fp group on the generators [ f1, f2 ]>
gap> Size(H);
2
gap> H := G / [ F.1 ];
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `/' on 2 arguments at ./lib/methsel2.g:250 called from
<function "HANDLE_METHOD_NOT_FOUND">( <arguments> )
 called from read-eval loop at *stdin*:5
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> quit;
gap> G := F / [ [ F.1, F.2^-1 ] ];
<fp group of size infinity on the generators [ f1, f2 ]>
gap> H := G / [ [ G.1^2, G.2 ] ];
<fp group on the generators [ f1, f2 ]>
gap> H := G / [ [ F.1, F.2] ];
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `/' on 2 arguments at ./lib/methsel2.g:250 called from
<function "HANDLE_METHOD_NOT_FOUND">( <arguments> )
 called from read-eval loop at *stdin*:7
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> quit;

This is the behaviour you want, isn't it? If one wanted to support G / [ F.1 ] one would (and should, as it makes explicit what one is doing) have to use the canonical homomorphism from F onto G (of which I don't know how to get hold of),

What does not work yet is the following

gap> D := DihedralGroup(IsPermGroup, 8);;
gap> G := D / [ (1,2,3,4)];

where the expected output then would be

gap> G := D / [ (1,2,3,4) ];
<pc group with 1 generators>

Which could be made to work by installing the appropriate method. I don't see right now how I can make <group> / <elmslist> generic though: If I try forming the (generic) quotient like this:

gap> FactorByClosureOfElms := function(G, elmslist)
             local N;
             N := NormalClosure(G, Subgroup(G, elmslist)); # <- this kicks off a Todd-Coxeter coset enumeration (for infinite index this doesn't terminate)
             return G / N; # <- and this is hence not reached.
end;

Maybe I am overlooking something?

fingolfin · 2017-11-16T11:52:54Z

I don't think that a generic method for FactorByClosureOfElms is possible that works efficiently in "all" cases. But one could (and should) install methods for it that deal with specific cases. So for fpgroups, one could simply install FactorGroupFpGroupByRels as a method.

markuspf · 2017-11-16T12:42:14Z

Max Horn writes:

I don't think that a generic method for FactorByClosureOfElms is possible that works efficiently in "all" cases. But one could (and should) install methods for it that deal with specific cases. So for fpgroups, one could simply install FactorGroupFpGroupByRels as a method.

You mean you want to introduce a new Operation "FactorByNormalClosureOfElms"?

fingolfin · 2017-11-16T18:14:34Z

No, what I mean is more like this: If you want/need a FactorByClosureOfElms, then I think the only sane way to do it is to make it an operation, because how to do it efficiently varies quite a bit depending on how the group is represented internally, and the generic case using a normal closure won't be useful in all of them, as you pointed out yourself :-)

markuspf · 2017-11-16T20:16:12Z

Max Horn writes:

No, what I mean is more like this: If *you* want/need a `FactorByClosureOfElms`, then I think the only sane way to do it is to make it an operation, because how to do it efficiently varies quite a bit depending on how the group is represented internally, and the generic case using a normal closure won't be useful in all of them, as you pointed out yourself :-)

Sorry for being obstinate: Say I make an operation `FactorByNormalClosureOfElms`, I would then install this as method for `\/` on `[IsGroup, IsElmsColl]`?

fingolfin · 2017-11-16T22:36:37Z

That sounds sensible to me

hulpke · 2017-11-16T22:42:34Z

@markuspf
`Word' just should have been product of elements or inverses, i.e. an evaluated word.
Thanks for updating!

olexandr-konovalov · 2017-11-25T12:04:36Z

I've started Jenkins test of this PR to check how it works with different packages loaded.

olexandr-konovalov · 2017-11-26T22:22:02Z

This will break teststandard, since If all packages are loaded, we will have two diffs (unless this will somehow vanish after rebase, but I doubt that):

########> Diff in /circa/scratch/gap-jenkins/workspace/GAP-pull-request-quickt\
est/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/standard/label/kovacs/GAP-pull-reque\
st-snapshot/tst/testinstall/grpfree.tst:66
# Input is:
rho := SemigroupCongruenceByGeneratingPairs(F, [[F.1, F.2]]);
# Expected output:
<semigroup congruence with 1 generating pairs>
# But found:
<semigroup congruence over <free group on the generators [ f1, f2 ]> with 
1 generating pairs>
########
########> Diff in /circa/scratch/gap-jenkins/workspace/GAP-pull-request-quickt\
est/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/standard/label/kovacs/GAP-pull-reque\
st-snapshot/tst/testinstall/grpfree.tst:68
# Input is:
G := F / rho;
# Expected output:
<quotient of Group( [ f1, f2 ] ) by SemigroupCongruence( [ [ f1, f2 ] ] )>
# But found:
<quotient of <semigroup congruence over <free group on the generators 
[ f1, f2 ]> with 1 generating pairs>>
########

Shall this be treated as an issue in the core system or in a package? Perhaps this comes from one of @james-d-mitchell's packages?

olexandr-konovalov

Marked this as "Request changes" what just means "feedback that must be addressed before merging". Not clear if changes are required in the core system or some package.

james-d-mitchell · 2017-11-27T13:02:28Z

@alex-konovalov we do have a method for displaying semigroup congruences in Semigroups, which might cause the diff. I would simply not display the return value in these tests.

james-d-mitchell

Looks ok to me, other than that one question.

james-d-mitchell · 2017-11-27T13:18:17Z

lib/semiquo.gi

@@ -51,7 +51,8 @@ function(cong)
    SetOne(Q, One(S)^QuotientSemigroupHomomorphism(Q));
  fi;

-  if HasGeneratorsOfSemigroup(S) then
+  if HasGeneratorsOfMagmaWithInverses(S) or
+     HasGeneratorsOfSemigroup(S) then
    Qgens:= List( GeneratorsOfSemigroup( S ),


Doesn't this assume that if HasGeneratorsOfMagmaWithInverses then we can obtain GeneratorsOfSemigroup? Is this safe?

Probably not. I'll check.

markuspf · 2017-11-27T14:15:58Z

I don't really like suppressing output, because it then removes any notion of testing whether the output (or the result) makes any sense.

I can't think of a better way to address this problem though.

james-d-mitchell · 2017-11-27T14:26:08Z

You could check essentially what is displayed:

IsQuotientSemigroup(blah) and Length(GeneratingPairsOfSemigroupCongruence(*)) = 1 and * = FreeGroup(whatever)

fingolfin · 2017-11-29T16:05:14Z

lib/semiquo.gi

+  # GeneratorsOfMagmaWithInverses.
+  # 
+  # We put this in, because groups do not set their 
+  # semigroup generators.


I don't understand the second paragraph. I.e. the implication "because" indicates is beyond me.

It describes the reason why this hack is there in the first place: groups do not know their semigroup generating set, they only know their GeneratorsOfMagmaWithInverses.

I don't know whether putting Semigroup and Monoid generating sets into Group constructors wreaks more havoc on the library than I want to fix right now.

Actually, after reading this added "explanation" I still do not quite understand what you mean. What is "this" in "We put this in" referring to: the check? Or the code inside the if/fi? Each interpretation seems valid to me (if one does not study resp. know the surrounding code well, at least). I first thought the latter, but now I think perhaps the former is meant?

After reading the surrounding code, and also the discussion in the PR, I think I now have a guess of what was meant, and wrote the following suggestion for an alternate comment based on that -- did I get it right?

Ensure GeneratorsOfSemigroup(Q) is set if possible, by mapping GeneratorsOfSemigroup(S).
Note that it is not sufficient to check HasGeneratorsOfSemigroup(S), because e. g. groups by default
have GeneratorsOfMagmaWithInverses set but not GeneratorsOfSemigroup. However, there are
GeneratorsOfSemigroup methods for domains knowing GeneratorsOfMagmaWithInverses or
GeneratorsOfMagma, thus the following check and code works correctly even for groups.

fingolfin · 2017-11-29T16:10:47Z

doc/ref/grpfp.xml

 Then a list of relators is constructed as words in the generators of the
-free group and is factored out to obtain the finitely presented group.
+free group, or a list of equations, that is, pairs of words in the generators
+of the free group, and is factored out to obtain the finitely presented group.


That sentence now is somewhat longish and it took me some time grok it. How about this?

Then either a list of relators is constructed as words in the generators of the free group,
or else a list of relations as pairs of words in the generators of the free group. In either case,
the quotient of the free group by the list is the finitely presented group with the given
relators respectively relations.

fingolfin · 2017-11-29T16:11:13Z

doc/ref/grpfp.xml

@@ -119,16 +123,19 @@ See also <Ref Func="SetReducedMultiplication"/>.
 <ManSection>
 <Meth Name="\/" Arg="F, rels"
 Label="for a free group and a list of elements"/>
+<Meth Name="\/" Arg="F, eqns"
+ Label="for a free group and a list of equations"/>


maybe replace "equations" by "pairs of elements"?

fingolfin · 2017-11-29T16:12:23Z

doc/ref/grpfp.xml

+Note that relations are entered as <E>relators</E>, i.e. as words in the
+generators of <A>F</A>, or as <E>equations</E>, i.e. as pairs of
+words in the generators of the group <A>F</A>.
+Note that relators and equations cannot be mixed.


Perhaps avoid using "Note that" twice? It's purely a filler anyway, both could be removed.

The semigroups package installs a different method to print congruences and quotients, so we only test some of the properties of the results, not the output itself.

markuspf · 2017-12-01T16:00:16Z

@alex-konovalov can you please check about your issue with this PR again and remove the blocker, if appropriate

fingolfin

Looks good to me now, thanks!

olexandr-konovalov · 2017-12-01T17:19:48Z

@markuspf I am re-running Jenkins test with the latest state of this PR - please bear with me till it's completed.

olexandr-konovalov · 2017-12-02T11:54:05Z

@markuspf happy with this now. I've seen some diffs in Jenkins, but they all look familiar and caused by earlier problems that are already fixed.

markuspf added kind: bug Issues describing general bugs, and PRs fixing them kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements labels Nov 10, 2017

markuspf added this to the GAP 4.9.0 milestone Nov 10, 2017

markuspf requested a review from james-d-mitchell November 10, 2017 15:03

fingolfin reviewed Nov 10, 2017

View reviewed changes

fingolfin requested a review from hulpke November 10, 2017 15:44

Install some methods to take quotients of free and fp groups

d97778f

We install methods to take quotients of finitely presented groups by lists of relations, and quotients of free and finitely presented groups by lists of equations (pairs of left hand side and right hand side).

markuspf force-pushed the some-fp-group-tweaks branch from 46c2855 to 2443b83 Compare November 10, 2017 16:28

hulpke requested changes Nov 13, 2017

View reviewed changes

hulpke approved these changes Nov 16, 2017

View reviewed changes

olexandr-konovalov requested changes Nov 27, 2017

View reviewed changes

james-d-mitchell reviewed Nov 27, 2017

View reviewed changes

markuspf force-pushed the some-fp-group-tweaks branch from 2443b83 to 2aa98e8 Compare November 27, 2017 15:51

fingolfin reviewed Nov 29, 2017

View reviewed changes

markuspf force-pushed the some-fp-group-tweaks branch from 2aa98e8 to 0552648 Compare November 29, 2017 16:47

markuspf added 2 commits November 30, 2017 09:57

Set semigroup generators of quotient of group by semigroup congruence

74c0c81

Add some tests.

4859aaf

The semigroups package installs a different method to print congruences and quotients, so we only test some of the properties of the results, not the output itself.

markuspf force-pushed the some-fp-group-tweaks branch 2 times, most recently from d644417 to 4bb6934 Compare November 30, 2017 10:06

Document quotients by equations

d157f53

markuspf force-pushed the some-fp-group-tweaks branch from 4bb6934 to d157f53 Compare December 1, 2017 16:08

fingolfin approved these changes Dec 1, 2017

View reviewed changes

olexandr-konovalov approved these changes Dec 2, 2017

View reviewed changes

olexandr-konovalov merged commit 697f87f into gap-system:master Dec 2, 2017

olexandr-konovalov changed the title ~~Some fp group tweaks~~ Fix recursion depth trap and other improvements for quotients of fp groups Jan 29, 2018

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

markuspf deleted the some-fp-group-tweaks branch February 19, 2018 15:43

Fix recursion depth trap and other improvements for quotients of fp groups #1884

Fix recursion depth trap and other improvements for quotients of fp groups #1884

Conversation

markuspf commented Nov 10, 2017 • edited Loading

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Nov 10, 2017

codecov bot commented Nov 10, 2017 • edited Loading

Codecov Report

stevelinton commented Nov 12, 2017

markuspf commented Nov 13, 2017

hulpke left a comment

Choose a reason for hiding this comment

markuspf commented Nov 13, 2017

markuspf commented Nov 13, 2017

fingolfin commented Nov 16, 2017

markuspf commented Nov 16, 2017 via email

fingolfin commented Nov 16, 2017

markuspf commented Nov 16, 2017 via email

fingolfin commented Nov 16, 2017

hulpke commented Nov 16, 2017

olexandr-konovalov commented Nov 25, 2017

olexandr-konovalov commented Nov 26, 2017

olexandr-konovalov left a comment

Choose a reason for hiding this comment

james-d-mitchell commented Nov 27, 2017

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

markuspf commented Nov 27, 2017

james-d-mitchell commented Nov 27, 2017

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

markuspf commented Dec 1, 2017

fingolfin left a comment

Choose a reason for hiding this comment

olexandr-konovalov commented Dec 1, 2017

olexandr-konovalov commented Dec 2, 2017

markuspf commented Nov 10, 2017 •

edited

Loading

codecov bot commented Nov 10, 2017 •

edited

Loading