Fix `ONanScottType` and introduce `RestrictedInverseGeneralMapping` #1937

hulpke · 2017-11-21T19:02:41Z

These are (besides minor enhancements) corrections that are needed for 4.9:

ONanScottType, Homomorphisms

codecov · 2017-11-22T03:28:30Z

Codecov Report

Merging #1937 into master will increase coverage by 0.01%.
The diff coverage is 85.57%.

@@            Coverage Diff             @@
##           master    #1937      +/-   ##
==========================================
+ Coverage   65.88%   65.89%   +0.01%     
==========================================
  Files         898      898              
  Lines      273240   273306      +66     
  Branches    12749    12775      +26     
==========================================
+ Hits       180020   180091      +71     
+ Misses      90406    90398       -8     
- Partials     2814     2817       +3

Impacted Files	Coverage Δ
lib/ctbl.gd	`70.83% <ø> (ø)`	⬆️
lib/mapprep.gi	`69.29% <0%> (-0.58%)`	⬇️
lib/fitfree.gi	`51.54% <0%> (ø)`	⬆️
lib/grppcext.gi	`50.36% <0%> (ø)`	⬆️
lib/ghomperm.gi	`90.43% <100%> (ø)`	⬆️
grp/simple.gi	`59.42% <100%> (ø)`	⬆️
lib/ghom.gi	`72.83% <88.88%> (+0.5%)`	⬆️
lib/grpperm.gi	`86.43% <92.06%> (+1.06%)`	⬆️
lib/queue.g	`66.4% <0%> (-3.2%)`	⬇️
src/hpc/thread.c	`45.41% <0%> (-0.6%)`	⬇️
... and 9 more

markuspf · 2017-12-01T06:23:35Z

lib/grpperm.gi

+    # so the group now is of type 3 or 4. Next we determine a simple socle
+    # factor and a direct product of all but one socle factor.
+    # simple socle factor.
+    stb:=Stabilizer(s,1);


Isn't this wrong if the socle's domain doesn't include 1?

gap> G := Group(GeneratorsOfGroup(PrimitiveGroup(3600,1)));; gap> ONanScottType(G^(1,3601)); "4b"

Yes. Thank you. Fixed.

olexandr-konovalov · 2017-12-01T14:47:24Z

This now breaks grpperm.tst:

########> 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/testextra/grpperm.tst:90
# Input is:
m:=MaximalSubgroupClassReps(w);;
# Expected output:
# But found:
Error, reached the pre-set memory limit
(change it with the -o command line option)
########
########> 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/testextra/grpperm.tst:91
# Input is:
Collected(List(m,x->Index(w,x)));
# Expected output:
[ [ 2, 3 ], [ 5, 1 ], [ 6, 1 ], [ 10, 1 ], [ 16, 1 ], [ 3125, 1 ], 
  [ 7776, 1 ], [ 100000, 1 ] ]
# But found:
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `ListOp' on 2 arguments
########
 2589404 ms (346566 ms GC) and 1.89TB allocated for grpperm.tst

The 2nd diff clearly follows from the 1st; seeing 1.89TB allocated at the end is very useful!

It also breaks manual examples, but that does not sound dangerous:

########> Diff in [ "./../../lib/ctbl.gd", 4366, 4390 ]
# Input is:
kernel:= KernelOfCharacter( irr[3] );
# Expected output:
Group([ (1,2)(3,4), (1,4)(2,3) ])
# But found:
Group([ (1,2)(3,4), (1,3)(2,4) ])
########
########> Diff in [ "./../../lib/ctbl.gd", 4366, 4390 ]
# Input is:
NormalSubgroupClassesInfo( tbl );
# Expected output:
rec( nsg := [ Group([ (1,2)(3,4), (1,4)(2,3) ]) ], 
  nsgclasses := [ [ 1, 3 ] ], nsgfactors := [  ] )
# But found:
rec( nsg := [ Group([ (1,2)(3,4), (1,3)(2,4) ]) ], 
  nsgclasses := [ [ 1, 3 ] ], nsgfactors := [  ] )
########

olexandr-konovalov · 2017-12-01T14:53:47Z

@hulpke furthermore with the current version of this PR, I have:

gap> for i in [1..33] do
> g:=PrimitiveGroup(3600,i);
> h:=Group(GeneratorsOfGroup(g));
> tg:=ONanScottType(g); th:=ONanScottType(h);
> Print(i, " : ", tg, " : ", th, " : " , tg = th, "\n" ); od;
1 : 3b : 3b : true
2 : 3b : 3b : true
3 : 3b : 3b : true
4 : 3b : 3b : true
5 : 3b : 3b : true
6 : 4c : 4a : false
7 : 4c : 4b : false
8 : 4c : 4a : false
9 : 4c : 4a : false
10 : 4c : 4b : false
11 : 4c : 4b : false
12 : 4c : 4b : false
13 : 4c : 4b : false
14 : 4c : 4b : false
15 : 4c : 4b : false
16 : 4c : 4a : false
17 : 4c : 4b : false
18 : 4c : 4b : false
19 : 4c : 4b : false
20 : 4c : 4b : false
21 : 4c : 4b : false
22 : 4c : 4b : false
23 : 4c : 4b : false
24 : 4c : 4b : false
25 : 4c : 4b : false
26 : 4c : 4b : false
27 : 4c : 4b : false
28 : 4c : 4b : false
29 : 4c : 4b : false
30 : 4c : 4c : true
31 : 4c : 4c : true
32 : 4c : 4c : true
33 : 4c : 4c : true

(compare with previous behaviour in #852).

hulpke · 2017-12-01T15:01:49Z

@alex-konovalov
I expected errors. I'll look at them later today.

olexandr-konovalov · 2017-12-01T17:18:03Z

@hulpke thanks. I guess this PR will require an interactive rebase before merging it in its final form.

hulpke · 2017-12-01T17:20:17Z

@alex-konovalov
I seem to be unable to produce the grpperm.tst error in the `additions' branch:

gap> SetAssertionLevel(2);
gap> Test("tst/testextra/grpperm.tst");

runs through in a few minutes. Should I call it differently?

olexandr-konovalov · 2017-12-01T17:22:28Z

@hulpke what if you start GAP with -o 1g as make teststandard does?

gap/Makefile.rules

Line 659 in 9a73578

    
           TESTGAP = $(abs_top_builddir)/bin/gap.sh --quitonbreak -b -m 100m -o 1g -q -x 80 -r -A $(GAPARGS)

hulpke · 2017-12-01T19:18:11Z

@alex-konovalov
Thanks. There was a minor inefficiency that got triggered badly by the homomorphism assertions. Fixed now.

olexandr-konovalov · 2017-12-02T18:24:05Z

@hulpke could be useful to add a test file calling ONanScottType on problematic groups given by their generators.

hulpke · 2017-12-03T17:19:44Z

@alex-konovalov
Yes, and an obvious base would be the primitive groups library (to avoid retyping permutations on 3000+ points). Is that guaranteed to work, even if the primitive groups library is a package? How much time should it spend?

olexandr-konovalov · 2017-12-03T17:29:02Z

@hulpke will work, because PrimGrp is required package.

ssiccha · 2017-12-04T12:00:51Z

For a discussion of the enhancements see #1982

hulpke · 2017-12-07T01:55:13Z

@alex-konovalov
I've added some tests to grpperm.tst will rebase before merge.

fingolfin

Looks good to me, esp. the new RestrictedInverseGeneralMapping (which also fixes the polycyclic tests, which now pass again, yay :-)

olexandr-konovalov · 2017-12-08T12:42:10Z

lib/ghomperm.gi

+    #            [ n + 1 .. n + k ], conperminv, One( Source( hom ) ), hom,
+    #            KernelOfMultiplicativeGeneralMapping );
+    #fi;
+


Perhaps there is no need to keep the commented out code above any more?

I can take it out -- it was there to allow for easy undo if issues would have come up.

olexandr-konovalov · 2017-12-08T13:05:55Z

lib/mapping.gd

+##
+#A  RestrictedInverseGeneralMapping( <map> )
+##
+##  <#GAPDoc Label="RestrictedInverseGeneralMapping">


Is there a matching Include statement somewhere in doc/ref/*.xml?

olexandr-konovalov · 2017-12-08T13:39:13Z

lib/grpperm.gd

+##  positive integer <A>e</A> such that <A>a^e=b</A>, or <K>false</A> if no such
+##  element exists.
+##  </Description>
+##  </ManSection>


Just to observe that this is not documented (fine with me, just in case).

Also, would it be better to return fail instead of false?

I'd prefer to stay with false for a valid "not a number" output, and keep fail for "calculation did not complete".

olexandr-konovalov · 2017-12-08T21:39:10Z

@hulpke thanks, looks good to me now. Will you do git rebase -i master to edit the PR to squash corrections into appropriate commits, or you would prefer me or @fingolfin to do this?

generator redundancy testing in perm groups. FIX: Need to place differently to satisfy load order.

This will resolve gap-system#944 and gap-system#1971 (also utilize stored inverse hom.)

This fixes gap-system#852 also changed manual example that was affected by prior change This includes: added enhancements suggested by ssicha, tests, manual include, minor cleanup.

hulpke · 2017-12-08T22:19:54Z

@alex-konovalov
I have rebased now (If I rebase earlier I worried the reviuew comments vanish).

What do we do with the data library, that will need updates as well?

olexandr-konovalov · 2017-12-08T22:28:56Z

@hulpke there is a PR for primitive groups library at gap-packages/primgrp#12 - looks good to me, I am going to merge it and release new version. Please have a look at it too and leave a comment if it looks good to you.

olexandr-konovalov · 2017-12-08T22:32:18Z

lib/permutat.g

@@ -722,6 +722,7 @@ InstallMethod( Order,
    [ IsPerm ],
    ORDER_PERM );

+
 #############################################################################


This file is included in this PR, but only because an empty line has been added? Could you revert it, please?

olexandr-konovalov · 2017-12-08T23:14:51Z

@hulpke said

I have rebased now (If I rebase earlier I worried the reviuew comments vanish).

I still can see them clicking on "Show outdated", so it's fine with me if you will amend commits and force-push in cases like that, instead of pushing a correcting commit, and then rebasing and force-pushing.

olexandr-konovalov · 2017-12-10T17:23:28Z

Checked again with gap-packages/primgrp#12, all works fine. I think we can merge this now.

olexandr-konovalov · 2017-12-10T21:46:44Z

I've released new PrimGrp 3.3.0 today. It will be picked up for testing tomorrow.

hulpke added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Nov 21, 2017

hulpke added this to the GAP 4.10.0 milestone Nov 21, 2017

hulpke force-pushed the additions branch from f5b6b37 to ec64432 Compare November 22, 2017 03:28

hulpke force-pushed the additions branch from ec64432 to 8f5aa5a Compare November 30, 2017 22:20

hulpke modified the milestones: GAP 4.10.0, GAP 4.9.0 Dec 1, 2017

hulpke removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Dec 1, 2017

hulpke changed the title ~~Next family of additions, so far only to do automated tests~~ Corrections of ONanScottType and Inverse hom. if not surjective Dec 1, 2017

markuspf reviewed Dec 1, 2017

View reviewed changes

hulpke force-pushed the additions branch from a1ad411 to 9d837a4 Compare December 1, 2017 19:16

ssiccha mentioned this pull request Dec 4, 2017

Rewrite ONanScottType #1982

Closed

hulpke force-pushed the additions branch from 9d837a4 to 53fb2c3 Compare December 7, 2017 01:53

olexandr-konovalov mentioned this pull request Dec 8, 2017

InverseGeneralMapping regression, revert 59097e248? #944

Closed

fingolfin approved these changes Dec 8, 2017

View reviewed changes

olexandr-konovalov requested changes Dec 8, 2017

View reviewed changes

olexandr-konovalov reviewed Dec 8, 2017

View reviewed changes

olexandr-konovalov approved these changes Dec 8, 2017

View reviewed changes

hulpke added 3 commits December 8, 2017 15:17

ENHANCE: Add utility function LogPerm and use it to speed up

935e988

generator redundancy testing in perm groups. FIX: Need to place differently to satisfy load order.

FIX: Introduce `RestrictedInverseGeneralMapping'.

f292927

This will resolve gap-system#944 and gap-system#1971 (also utilize stored inverse hom.)

FIX: Rewrote ONanScottType

e69615d

This fixes gap-system#852 also changed manual example that was affected by prior change This includes: added enhancements suggested by ssicha, tests, manual include, minor cleanup.

hulpke force-pushed the additions branch from 67756cd to e69615d Compare December 8, 2017 22:18

olexandr-konovalov reviewed Dec 8, 2017

View reviewed changes

Removed blank line

6105a68

olexandr-konovalov merged commit f47ca5b into gap-system:master Dec 10, 2017

olexandr-konovalov changed the title ~~Corrections of ONanScottType and Inverse hom. if not surjective~~ Fix ONanScottType and introduce RestrictedInverseGeneralMapping. Jan 29, 2018

olexandr-konovalov changed the title ~~Fix ONanScottType and introduce RestrictedInverseGeneralMapping.~~ Fix ONanScottType and introduce RestrictedInverseGeneralMapping 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `ONanScottType` and introduce `RestrictedInverseGeneralMapping` #1937

Fix `ONanScottType` and introduce `RestrictedInverseGeneralMapping` #1937

hulpke commented Nov 21, 2017 •

edited

Loading

codecov bot commented Nov 22, 2017 •

edited

Loading

markuspf Dec 1, 2017

markuspf Dec 1, 2017

hulpke Dec 1, 2017

olexandr-konovalov commented Dec 1, 2017 •

edited

Loading

olexandr-konovalov commented Dec 1, 2017 •

edited

Loading

hulpke commented Dec 1, 2017

olexandr-konovalov commented Dec 1, 2017

hulpke commented Dec 1, 2017

olexandr-konovalov commented Dec 1, 2017

hulpke commented Dec 1, 2017

olexandr-konovalov commented Dec 2, 2017

hulpke commented Dec 3, 2017

olexandr-konovalov commented Dec 3, 2017

ssiccha commented Dec 4, 2017 •

edited

Loading

hulpke commented Dec 7, 2017 •

edited

Loading

fingolfin left a comment

olexandr-konovalov Dec 8, 2017

hulpke Dec 8, 2017

olexandr-konovalov Dec 8, 2017

hulpke Dec 8, 2017

olexandr-konovalov Dec 8, 2017

hulpke Dec 8, 2017

olexandr-konovalov commented Dec 8, 2017

hulpke commented Dec 8, 2017

olexandr-konovalov commented Dec 8, 2017

olexandr-konovalov Dec 8, 2017

olexandr-konovalov commented Dec 8, 2017

olexandr-konovalov commented Dec 10, 2017

olexandr-konovalov commented Dec 10, 2017

Fix ONanScottType and introduce RestrictedInverseGeneralMapping #1937

Fix ONanScottType and introduce RestrictedInverseGeneralMapping #1937

Conversation

hulpke commented Nov 21, 2017 • edited Loading

codecov bot commented Nov 22, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olexandr-konovalov commented Dec 1, 2017 • edited Loading

olexandr-konovalov commented Dec 1, 2017 • edited Loading

hulpke commented Dec 1, 2017

olexandr-konovalov commented Dec 1, 2017

hulpke commented Dec 1, 2017

olexandr-konovalov commented Dec 1, 2017

hulpke commented Dec 1, 2017

olexandr-konovalov commented Dec 2, 2017

hulpke commented Dec 3, 2017

olexandr-konovalov commented Dec 3, 2017

ssiccha commented Dec 4, 2017 • edited Loading

hulpke commented Dec 7, 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

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

olexandr-konovalov commented Dec 8, 2017

hulpke commented Dec 8, 2017

olexandr-konovalov commented Dec 8, 2017

Choose a reason for hiding this comment

olexandr-konovalov commented Dec 8, 2017

olexandr-konovalov commented Dec 10, 2017

olexandr-konovalov commented Dec 10, 2017

Fix `ONanScottType` and introduce `RestrictedInverseGeneralMapping` #1937

Fix `ONanScottType` and introduce `RestrictedInverseGeneralMapping` #1937

hulpke commented Nov 21, 2017 •

edited

Loading

codecov bot commented Nov 22, 2017 •

edited

Loading

olexandr-konovalov commented Dec 1, 2017 •

edited

Loading

olexandr-konovalov commented Dec 1, 2017 •

edited

Loading

ssiccha commented Dec 4, 2017 •

edited

Loading

hulpke commented Dec 7, 2017 •

edited

Loading