Update ThrowAwayFixedPoints #191

ssiccha · 2020-10-23T15:21:35Z

Also lets ThrowAwayFixedPoints create a homomorphism if the input group
is primitive and has fixed points.

Fixes #190.

codecov · 2020-10-23T15:29:15Z

Codecov Report

Merging #191 (6d37718) into master (f4cf7d3) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 6d37718 differs from pull request most recent head bca52ef. Consider uploading reports for the commit bca52ef to get more accurate results

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
- Coverage   77.76%   77.69%   -0.08%     
==========================================
  Files          37       37              
  Lines       17497    17342     -155     
==========================================
- Hits        13606    13473     -133     
+ Misses       3891     3869      -22

Impacted Files	Coverage Δ
gap/base/methsel.gd	`100.00% <ø> (ø)`
gap/perm.gi	`97.74% <100.00%> (-0.03%)`	⬇️
gap/perm/largebase.gi	`82.44% <100.00%> (+0.06%)`	⬆️
gap/base/hack.g	`82.14% <0.00%> (-7.86%)`	⬇️
gap/base/recognition.gi	`69.51% <0.00%> (-1.40%)`	⬇️
gap/projective/d247.gi	`75.07% <0.00%> (-1.28%)`	⬇️
gap/projective/almostsimple.gi	`65.60% <0.00%> (-0.77%)`	⬇️
gap/projective.gi	`88.41% <0.00%> (-0.66%)`	⬇️
gap/projective/findnormal.gi	`77.74% <0.00%> (-0.37%)`	⬇️
gap/matrix.gi	`91.06% <0.00%> (-0.09%)`	⬇️
... and 9 more

ssiccha · 2020-10-23T15:34:14Z

So, on the upside this fixes #190. On the downside recognition of a large base primitive groups with fixed points takes 20 seconds compared to 2 seconds for an isomorphic group but without fixed points.

Here is an example run:

gap> S23OnSets := Action(SymmetricGroup(23), Combinations([1 .. 23], 2), OnSets);;
gap> jellyGroup2 := WreathProductProductAction(S23OnSets, Group((1,2)));;
gap> n := NrMovedPoints(jellyGroup2);
64009
gap> ri2 := RecogniseGroup(jellyGroup2); time;
F pts= 64009  0        
<recoginfo LargeBasePrimitive
 F:<recoginfo Imprimitive
    F:<recoginfo Pcgs Size=2>
    K:<recoginfo BalTreeForBlocks
       F:<recoginfo Giant AlmostSimple Size=25852016738884976640000>
       K:<recoginfo ThrowAwayFixedPoints
          F:<recoginfo Giant AlmostSimple Size=25852016738884976640000>
          K:<trivial kernel>>>
 K:<trivial kernel>
1891
gap> random := Random(SymmetricGroup(5 * QuoInt(n,2)));;
gap> jellyGroup2Conj := jellyGroup2 ^ random;;
gap> ri2Conj := RecogniseGroup(jellyGroup2Conj); time;
F pts=160018  0         
<recoginfo ThrowAwayFixedPoints
 F:<recoginfo LargeBasePrimitive
    F:<recoginfo Imprimitive
       F:<recoginfo Pcgs Size=2>
       K:<recoginfo BalTreeForBlocks
          F:<recoginfo Giant AlmostSimple Size=25852016738884976640000>
          K:<recoginfo ThrowAwayFixedPoints
             F:<recoginfo Giant AlmostSimple Size=25852016738884976640000>
             K:<trivial kernel>>>
    K:<trivial kernel>
 K:<trivial kernel>
22211

I'm looking into a profile now and it seems like two things make this so slow

the IsPrimitive check I added is super expensive because computing the blocks takes ages
the multiplication of group elements with memory becomes very expensive, in the sense of that it gets executed very often

ssiccha · 2020-10-23T16:48:13Z

Hm, apparently the IsPrimitive call in the second level of the tree, that is directly after applying ThrowAwayFixedPoints for the first time, takes super long.

fingolfin · 2020-11-04T12:54:38Z

gap/perm.gi

-      if 2*n > Maximum(l) or 3*n > LargestMovedPoint(G) then  # we do nothing
+      largest := LargestMovedPoint(G);
+      if (2*n > Maximum(l) or 3*n > largest)
+              and not (IsPrimitive(G) and largest > n)


It's just too expensive to check IsPrimitive.

Instead we should either...

enhance the large base group to also check for fixed points and throw them away (either with its own code or by forcing a FindHomMethodsPerm.ThrowAwayFixedPoints step with this limit removed.

improve the large base group case to not require that MovedPoints(G) = [1..LargestMovedPoints(G)]

@ssiccha any plans to revisit this?

ssiccha · 2020-12-12T18:41:13Z

@fingolfin I just pushed a version that I think does the trick. The tests I added take roughly 17 seconds on my machine. Do you think that is too much for the quick testsuite?

ssiccha · 2020-12-15T22:47:58Z

Hmm, does this brake something in tst/working/quick/ProjLowIndex.tst?

ssiccha · 2020-12-17T19:37:10Z

The test-suite ran into a failure in ProjLowIndex.tst. I was able to reproduce that on my machine by executing tst/testquick.g. However, I also ran ProjLowIndex.tst a thousand times both with the current master branch and with this PR just to make sure, that this PR does not by some weird quirk introduce a regression. With this PR the test fails three out of a thousand times. In master it fails four out of a thousand times.

So by a big coincidence, when arriving at ProjLowIndex.tst in the test suite this PR exactly hit a random state, such that the group in ProjLowIndex.tst is recognized incorrectly.

I have thus changed the test in PermLargeBasePrimitive.tst to use a smaller group. Which has the benefit that a) the test runs quicker and b) ProjLowIndex.tst also passes again.

fingolfin

Seems sensible. Left some minor comments

gap/perm/largebase.gi

tst/working/quick/PermLargeBasePrimitive.tst

ssiccha · 2021-03-31T11:32:12Z

@fingolfin I have adjusted and integrated your suggestion into the documentation of LargeBasePrimitive. I've also added a short explanation to the documentation of ThrowAwayFixedPoints. Could you have a look again to see whether it is now clear how large base primitive groups with fixed points are handled?

fingolfin

Fine in principle, thank you! I do have some suggestions for the text, though.

gap/perm.gi

fingolfin · 2021-04-07T10:31:28Z

gap/perm.gi

-#! <K>NeverApplicable</K> if it does not define a homomorphism indicating that it will
-#! never succeed.
+#! or  <M>n</M> is at most half of the number of points on which
+#! <A>G</A> is stored internally by &GAP;.


Perhaps this should be explained a bit more, but then it is a bit technical... Perhaps what I wrote above could be extended... something along these lines:

The reason we do not just always discard fixed points is that it also incurs a cost, so we only try to do it when it seems worthwhile to do so.

To explain what was meant above by the ration of fixed points to moved points, suppose the group G is
not known to be primitive. Then we still apply this method if one of the following two criterion is met:

the largest moved point of G is three or more times larger than the number n of actually moved points

there is a generator of G whose internal storage reserves space for a number of moved points which is two or more times larger than n (note that even for a simple transposition (1,2), for technical reasons it can happen that GAP internally stores it with the same size as a permutation moving a million points; this is wasteful, and the second criterion tries to deal with this)

I agree that that's a bit technical. So I'll put it into a source comment but not into the manual.

fingolfin · 2021-04-07T10:33:28Z

gap/perm/largebase.gi

@@ -526,6 +538,7 @@ FindHomMethodsPerm.LargeBasePrimitive :=
    fi;
    RECOG.SetPseudoRandomStamp(grp,"Jellyfish");
    res := RECOG.AllJellyfish(grp);
+    RECOG.SetPseudoRandomStamp(grp,"PseudoRandom");


Why not this? (Honest question, I don't know)

Suggested change

RECOG.SetPseudoRandomStamp(grp,"PseudoRandom");

RECOG.SetPseudoRandomStamp(grp,"LargeBasePrimitive");

LargeBasePrimitive is more or less a wrapper for AllJellyfish. After the call to AllJellyfish the actual computations are done and now we're just managing stuff. Thus here I "reset" the stamp.

Yeah, I really do not like how we handle this whole RandomStamp business currently, see #261 for an idea how to improve this.

Now ThrowAwayFixedPoints also prunes fixed points if the input group knows that it is primitive and has fixed points. If the input group does not know whether it is primitive yet but has fixed points, then ThrowAwayFixedPoints now returns NotEnoughInformation. Adds a comment in LargeBasePrimitive which explains how this fixes gap-packages#190. That is this commit lets recog handle primitive groups which can be recognized by LargeBasePrimitive, except for the fact that they have fixed points. Also changes the tests in PermLargeBasePrimitive.tst to make them run a bit faster. Fixes gap-packages#190. Co-authored-by: Max Horn <[email protected]>

Co-authored-by: Max Horn <[email protected]>

ssiccha

I've added a commit with your suggestions and one with some minor changes by me. I'll squash everything when putting this into master.

ssiccha · 2021-04-07T15:04:49Z

gap/perm/largebase.gi

@@ -526,6 +538,7 @@ FindHomMethodsPerm.LargeBasePrimitive :=
    fi;
    RECOG.SetPseudoRandomStamp(grp,"Jellyfish");
    res := RECOG.AllJellyfish(grp);
+    RECOG.SetPseudoRandomStamp(grp,"PseudoRandom");


LargeBasePrimitive is more or less a wrapper for AllJellyfish. After the call to AllJellyfish the actual computations are done and now we're just managing stuff. Thus here I "reset" the stamp.

Yeah, I really do not like how we handle this whole RandomStamp business currently, see #261 for an idea how to improve this.

ssiccha · 2021-04-07T15:06:13Z

gap/perm.gi

-#! <K>NeverApplicable</K> if it does not define a homomorphism indicating that it will
-#! never succeed.
+#! or  <M>n</M> is at most half of the number of points on which
+#! <A>G</A> is stored internally by &GAP;.


I agree that that's a bit technical. So I'll put it into a source comment but not into the manual.

ssiccha added Do not merge enhancement labels Oct 23, 2020

ssiccha self-assigned this Oct 23, 2020

ssiccha requested a review from fingolfin October 23, 2020 15:21

ssiccha mentioned this pull request Oct 23, 2020

Large Base recognition fails although it shouldn't #190

Closed

fingolfin requested changes Nov 4, 2020

View reviewed changes

fingolfin closed this Dec 11, 2020

fingolfin reopened this Dec 11, 2020

ssiccha force-pushed the ss/update-ThrowAwayFixedPoints branch from 585d73d to 5b648ce Compare December 12, 2020 18:39

ssiccha removed the Do not merge label Dec 17, 2020

ssiccha force-pushed the ss/update-ThrowAwayFixedPoints branch from 5b648ce to 27e703b Compare December 17, 2020 14:13

fingolfin approved these changes Jan 13, 2021

View reviewed changes

gap/perm/largebase.gi Outdated Show resolved Hide resolved

tst/working/quick/PermLargeBasePrimitive.tst Outdated Show resolved Hide resolved

fingolfin closed this Feb 15, 2021

fingolfin reopened this Feb 15, 2021

ssiccha force-pushed the ss/update-ThrowAwayFixedPoints branch 2 times, most recently from 90e7a3f to 9510bd5 Compare March 31, 2021 11:28

ssiccha force-pushed the ss/update-ThrowAwayFixedPoints branch from 9510bd5 to 4f23e90 Compare March 31, 2021 11:33

fingolfin approved these changes Apr 7, 2021

View reviewed changes

ssiccha and others added 3 commits April 7, 2021 17:13

Changes from review

4dae588

Co-authored-by: Max Horn <[email protected]>

Changes by sergio

bca52ef

ssiccha force-pushed the ss/update-ThrowAwayFixedPoints branch from 806b9dc to bca52ef Compare April 7, 2021 15:13

ssiccha commented Apr 7, 2021

View reviewed changes

ssiccha merged commit 2f550bb into gap-packages:master Apr 20, 2021

ssiccha deleted the ss/update-ThrowAwayFixedPoints branch April 20, 2021 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ThrowAwayFixedPoints #191

Update ThrowAwayFixedPoints #191

ssiccha commented Oct 23, 2020

codecov bot commented Oct 23, 2020 •

edited

Loading

ssiccha commented Oct 23, 2020 •

edited

Loading

ssiccha commented Oct 23, 2020

fingolfin Nov 4, 2020

fingolfin Dec 11, 2020

ssiccha commented Dec 12, 2020

ssiccha commented Dec 15, 2020

ssiccha commented Dec 17, 2020

fingolfin left a comment

ssiccha commented Mar 31, 2021

fingolfin left a comment

fingolfin Apr 7, 2021

ssiccha Apr 7, 2021

fingolfin Apr 7, 2021

ssiccha Apr 7, 2021

ssiccha left a comment

ssiccha Apr 7, 2021

ssiccha Apr 7, 2021

	RECOG.SetPseudoRandomStamp(grp,"PseudoRandom");
	RECOG.SetPseudoRandomStamp(grp,"LargeBasePrimitive");

Update ThrowAwayFixedPoints #191

Update ThrowAwayFixedPoints #191

Conversation

ssiccha commented Oct 23, 2020

codecov bot commented Oct 23, 2020 • edited Loading

Codecov Report

ssiccha commented Oct 23, 2020 • edited Loading

ssiccha commented Oct 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssiccha commented Dec 12, 2020

ssiccha commented Dec 15, 2020

ssiccha commented Dec 17, 2020

fingolfin left a comment

Choose a reason for hiding this comment

ssiccha commented Mar 31, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssiccha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 23, 2020 •

edited

Loading

ssiccha commented Oct 23, 2020 •

edited

Loading