New Implementation of MTC #843

hulpke · 2016-07-01T21:02:20Z

There has been (in #302) a bug reported in the implementation of GAP's MTC. This is essentially still GAP3 code that was written by Volkmar Felsch in the early 1990s (and the bug was present in GAP 3).

The existing code is hard to understand and to debug as it moves all interesting work into a big kernel function `MakeConsequences'. It also is limited by sticking to some aspects (e.g. decomposition tree) of the MTC that stem from the 1970s and were in response to limited memory at that time. Besides the difficulty of debugging the code, it also makes it hard to experiment with it.

This PR contains a new from scratch implementation of a MTC (and also a plain TC) based on the description in the Handbook of CGT. It uses the fact that we have a memory manager and thus can be more flexible in handling lists.(For example, it seems to be easier to index the coset table by the numbers of the generators, even if it leaves gaps.)
It deliberately only keeps the scanning in a (very small) kernel function to allow for access to all other aspects of the code to allow, for example, easy implementation of other strategies.

Also included is use of this new code for (appropriate) fp group homomorphisms, this fixes #302.

Compared with the ordinary TC in GAP it currently is still a bit slower, but not by magnitudes. (The proposal keeps the existing ordinary TC and RRS in place and only concerns the MTC.)

It allows for using the strategy (that was implemented in the ITC package) of using collapses of a prior TC run to guide the definition process. Doing so can reduce the number of cosets to be defined very substantially and thus improve the result of the presentation produced by the MTC. (Doing so avoids the problem of the MTC choking in the generator elimination stage, which was a frequent occurrence and made it unusable in most cases.)

The functionality is exercised by existing tests, a few specific manual examples will be affected (and have not been changed).

The old MTC code is not yet removed, though it will not run any longer.

markuspf · 2016-07-01T21:07:04Z

Very nice!

I think @james-d-mitchell or one of his students has also implemented a
Todd-Coxeter variant for semigroups recently (in C).

codecov-io · 2016-07-01T22:18:58Z

Current coverage is 48.92% (diff: 80.62%)

Merging #843 into master will decrease coverage by 0.23%

@@             master       #843   diff @@
==========================================
  Files           422        422          
  Lines        233138     233842   +704   
  Methods        3479       3480     +1   
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits         114618     114417   -201   
- Misses       118520     119425   +905   
  Partials          0          0

Powered by Codecov. Last update 651e337...257e7a0

markuspf · 2016-07-05T15:40:47Z

Maybe @fingolfin can have a quick look, then we can merge this?

fingolfin · 2016-07-05T15:44:34Z

lib/ghomfp.gi

+ # fi;
+ # as we add homomorphism specific entries, lets be safe and copy.
+ # aug:=CopiedAugmentedCosetTable(aug);
+
 TrySecondaryImages(aug);


What is this newly added but commented out code about? Is this a deadend? Then it should be removed. Is it a place where a future extension is planned? Then it would be good to state that.

Otherwise, we'll just drag this around forever, and in years somebody will wonder what it's about,but unable to find out details...

Oh wait, I now realize that this is part of the old implementation, which has been commented out (I read the diff incorrectly before).

But that doesn't change the general gist of my assesment: I'd prefer to remove this (if somebody wants the old code it is in the repo), or at least add an explanatory commend: "Code for the old MTC starts here:" and later a corresponding "... end here". Though I don't know who would benefit from carrying around this commented out code?

fingolfin · 2016-07-05T16:10:27Z

Caveat: I have not yet actually tried the new code in this PR.

However, overall, I think it's absolutely great that @hulpke did this! Thanks a ton!

While I'd personally wish there were a few more comments, the fact that this very closely follows the presentation in the Handbook should make it much easier to maintain this code in the future. The extra comments I'd hope for mostly would be about explaining a few of the implementation choices. But that's not a blocker to me at all, and I am confident that if need be, we can (a) ask Alexander, and (b) even if Alexander were not around, reverse engineer it with help of the Handbook.

So, assuming that this code does not incur a major performance regression, I think it's an absolute nobrainer to merge it -- it should be a much better baseline than what we have right now.

That said, of course some more tests, and in particular one that tests for #302 (and verifies that it is fixed by this PR) would be swell... And also a few more comments as suggested by my review comments... but none of these are serious blockers from my point of view, and could be added later on.

markuspf · 2016-07-05T16:23:07Z

Wow thanks, thats much more thorough that the "quick look" I suggested, thanks for that! I did try the code "by hand" and didn't notice any problems. That is of course not really a principled approach to testing...

hulpke · 2016-07-05T16:58:53Z

@fingolfin
Thank you for your extensive comments. Assuming that there is agreement to replace the existing code (for MTC -- the plain TC is slower than the existing and probably would need more kernel support to catch up), which is really the intention behind having this as a PR relatively early, I'd be happy to add more comments.

So far the ``old'' code is left in to see clearly what changed. If there is agreement to only rely on git for this change, I'd be happy to take it out as well.

What also has not yet been implemented (and what I will do if there is agreement) is the special 1-generator version of the MTC used for `Size':

Calculate index of cyclic subgroup
Rewrite presentation, noting that with 1 generator the cofactors don't get abysmal
Calculate the order of the cyclic subgroup from the 1-generator presentation.

With that replaced, one can remove all of the ancient MTC, including its kernel functions.

hulpke · 2016-07-07T20:21:21Z

In the current version all uses of the old Mtc are removed (as is the actual library function). Subgroup abelianization and cyclic subgroup presentations are handled by special variants of the Mtc.

This allows for more flexibility in use and experimentation, as almost all code is in the library. It also will fix the problem of gap-system#302.

This fixes the previously reported bug. Close gap-system#302

by possibly not optimizing.

Also use new TC for cyclic index subgroup when computing size. Quiet option.

Removed library code for old version that is now obsolete.

olexandr-konovalov · 2016-08-04T16:11:42Z

@hulpke I've tested manual examples - does the new output look OK to you?

============================================================
########> Diff in [ "./../../lib/sgpres.gd", 506, 511 ]
# Input is:
p := PresentationSubgroupMtc( g, u );
# Expected output:
#I  there are 3 generators and 4 relators of total length 12
#I  there are 2 generators and 3 relators of total length 14
<presentation with 2 gens and 3 rels of total length 14>
# But found:
<presentation with 2 gens and 3 rels of total length 14>
########
########> Diff in [ "./../../lib/tietze.gd", 407, 467 ]
# Input is:
P := PresentationSubgroupMtc( G, H );;
# Expected output:
#I  index = 240  total = 4737  max = 4507
#I  MTC defined 2 primary and 4444 secondary subgroup generators
#I  there are 246 generators and 617 relators of total length 2893
#I  calling DecodeTree
#I  there are 114 generators and 385 relators of total length 1860
#I  there are 69 generators and 294 relators of total length 1855
#I  there are 43 generators and 235 relators of total length 2031
#I  there are 35 generators and 207 relators of total length 2348
#I  there are 25 generators and 181 relators of total length 3055
#I  there are 19 generators and 165 relators of total length 3290
#I  there are 20 generators and 160 relators of total length 5151
#I  there are 23 generators and 159 relators of total length 8177
#I  there are 25 generators and 159 relators of total length 12241
#I  there are 29 generators and 159 relators of total length 18242
#I  there are 34 generators and 159 relators of total length 27364
#I  there are 38 generators and 159 relators of total length 41480
#I  there are 41 generators and 159 relators of total length 62732
#I  there are 45 generators and 159 relators of total length 88872
#I  there are 46 generators and 159 relators of total length 111092
#I  there are 44 generators and 155 relators of total length 158181
#I  there are 32 generators and 155 relators of total length 180478
#I  there are 7 generators and 133 relators of total length 29897
#I  there are 4 generators and 119 relators of total length 28805
#I  there are 3 generators and 116 relators of total length 35209
#I  there are 2 generators and 111 relators of total length 25658
#I  there are 2 generators and 111 relators of total length 22634
# But found:
#I  there are 3 generators and 141 relators of total length 9773
#I  there are 3 generators and 136 relators of total length 5297
#I  there are 2 generators and 13 relators of total length 182
#I  there are 2 generators and 8 relators of total length 84
#I  there are 2 generators and 7 relators of total length 64
#I  there are 2 generators and 6 relators of total length 56
########
########> Diff in [ "./../../lib/tietze.gd", 407, 467 ]
# Input is:
TzGoGo( P );
# Expected output:
#I  there are 2 generators and 108 relators of total length 11760
#I  there are 2 generators and 95 relators of total length 6482
#I  there are 2 generators and 38 relators of total length 1464
#I  there are 2 generators and 8 relators of total length 116
#I  there are 2 generators and 7 relators of total length 76
#I  there are 2 generators and 6 relators of total length 66
#I  there are 2 generators and 6 relators of total length 52
# But found:
########
########> Diff in [ "./../../lib/tietze.gd", 407, 467 ]
# Input is:
TzPrintGenerators( P );
# Expected output:
#I  1.  _x1   26 occurrences
#I  2.  _x2   26 occurrences
# But found:
#I  1.  _x1   28 occurrences
#I  2.  _x2   28 occurrences
########
########> Diff in [ "./../../lib/tietze.gd", 407, 467 ]
# Input is:
TzPrint( P );
# Expected output:
#I  generators: [ _x1, _x2 ]
#I  relators:
#I  1.  3  [ 1, 1, 1 ]
#I  2.  3  [ 2, 2, 2 ]
#I  3.  8  [ 2, -1, 2, -1, 2, -1, 2, -1 ]
#I  4.  8  [ 2, 1, 2, 1, 2, 1, 2, 1 ]
#I  5.  14  [ -1, -2, 1, 2, 1, -2, -1, 2, 1, -2, -1, -2, 1, 2 ]
#I  6.  16  [ 1, 2, 1, -2, 1, 2, 1, -2, 1, 2, 1, -2, 1, 2, 1, -2 ]
# But found:
#I  generators: [ _x1, _x2 ]
#I  relators:
#I  1.  3  [ 2, 2, 2 ]
#I  2.  3  [ 1, 1, 1 ]
#I  3.  8  [ 2, -1, 2, -1, 2, -1, 2, -1 ]
#I  4.  8  [ 1, 2, 1, 2, 1, 2, 1, 2 ]
#I  5.  14  [ -1, -2, -1, 2, 1, -2, -1, 2, 1, 2, -1, -2, 1, 2 ]
#I  6.  20  [ -2, 1, -2, -1, 2, -1, -2, -1, 2, 1, 2, -1, 2, 1, 2, -1, 
  -2, -1, 2, -1 ]
########
########> Diff in [ "./../../lib/tietze.gd", 1628, 1639 ]
# Input is:
TzPrintOptions( P );
# Expected output:
#I  protected          = 0
#I  eliminationsLimit  = 100
#I  expandLimit        = 150
#I  generatorsLimit    = 0
#I  lengthLimit        = 2147483647
#I  loopLimit          = infinity
#I  printLevel         = 1
#I  saveLimit          = 10
#I  searchSimultaneous = 20
# But found:
#I  protected          = 2
#I  eliminationsLimit  = 100
#I  expandLimit        = 150
#I  generatorsLimit    = 0
#I  lengthLimit        = 2147483647
#I  loopLimit          = infinity
#I  printLevel         = 1
#I  saveLimit          = 10
#I  searchSimultaneous = 20
########
=========OUTPUT END: testmanuals with autoloaded packages=========

hulpke · 2016-08-05T12:43:05Z

@alex-konovalov
Yes, these manual changes are all harmless and a result of having a different coset enumeration.

olexandr-konovalov · 2016-08-05T17:54:45Z

@hulpke thanks - and even this one, where the output is missing completely?

########> Diff in [ "./../../lib/tietze.gd", 407, 467 ]
# Input is:
TzGoGo( P );
# Expected output:
#I  there are 2 generators and 108 relators of total length 11760
#I  there are 2 generators and 95 relators of total length 6482
#I  there are 2 generators and 38 relators of total length 1464
#I  there are 2 generators and 8 relators of total length 116
#I  there are 2 generators and 7 relators of total length 76
#I  there are 2 generators and 6 relators of total length 66
#I  there are 2 generators and 6 relators of total length 52
# But found:
########

hulpke mentioned this pull request Jul 1, 2016

Identity automorphism acting nontrivially #302

Closed

hulpke force-pushed the newmtc branch from d769834 to 3728bec Compare July 1, 2016 21:28

fingolfin reviewed Jul 5, 2016
View reviewed changes

hulpke force-pushed the newmtc branch 3 times, most recently from 03f18bc to 85b343f Compare July 7, 2016 20:18

hulpke added 6 commits August 4, 2016 06:52

NEW: Added kernel function TC_QUICK_SCAN for new MTC.

770ddab

NEW: New coset enumerator for MTC with less kernel dependence.

b5ac9f1

This allows for more flexibility in use and experimentation, as almost all code is in the library. It also will fix the problem of gap-system#302.

NEW: Fp Group GHBI uses new TC.

f0d660d

This fixes the previously reported bug. Close gap-system#302

NEW: When calling IsomorphismFpGroupByGenerators force same Nr. of gens

4c280f7

by possibly not optimizing.

NEW: Variant for MTC by cyclic subgroup -- use for group order.

629c3b1

Also use new TC for cyclic index subgroup when computing size. Quiet option.

NEW: Abelian version of MTC. Use in `AbelianInvariants' for subgp.

257e7a0

Removed library code for old version that is now obsolete.

hulpke force-pushed the newmtc branch from 85b343f to 257e7a0 Compare August 4, 2016 12:55

olexandr-konovalov added this to the GAP 4.9.0 milestone Aug 4, 2016

hulpke merged commit 5ccbeaa into gap-system:master Aug 4, 2016

fingolfin mentioned this pull request Sep 7, 2017

Release notes for GAP 4.9 #1699

Closed

hulpke mentioned this pull request Sep 18, 2018

FIX: Bad Memory inefficiency in new MTC. #2812

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Implementation of MTC #843

New Implementation of MTC #843

hulpke commented Jul 1, 2016

markuspf commented Jul 1, 2016

codecov-io commented Jul 1, 2016 •

edited

Loading

markuspf commented Jul 5, 2016

fingolfin Jul 5, 2016

fingolfin Jul 5, 2016

fingolfin commented Jul 5, 2016

markuspf commented Jul 5, 2016

hulpke commented Jul 5, 2016 •

edited

Loading

hulpke commented Jul 7, 2016

olexandr-konovalov commented Aug 4, 2016

hulpke commented Aug 5, 2016

olexandr-konovalov commented Aug 5, 2016

New Implementation of MTC #843

New Implementation of MTC #843

Conversation

hulpke commented Jul 1, 2016

markuspf commented Jul 1, 2016

codecov-io commented Jul 1, 2016 • edited Loading

Current coverage is 48.92% (diff: 80.62%)

markuspf commented Jul 5, 2016

fingolfin Jul 5, 2016

Choose a reason for hiding this comment

fingolfin Jul 5, 2016

Choose a reason for hiding this comment

fingolfin commented Jul 5, 2016

markuspf commented Jul 5, 2016

hulpke commented Jul 5, 2016 • edited Loading

hulpke commented Jul 7, 2016

olexandr-konovalov commented Aug 4, 2016

hulpke commented Aug 5, 2016

olexandr-konovalov commented Aug 5, 2016

codecov-io commented Jul 1, 2016 •

edited

Loading

hulpke commented Jul 5, 2016 •

edited

Loading