Fixes/Improvements that should make the 4.9 release #1809

hulpke · 2017-10-24T21:23:43Z

Bugfix and efficiency fixes that should still go into 4.9, together with minor interface cleanup of internal functions to avoid releasing code with outdated interface

Update (@alex-konovalov): this fixes #1774 and #1819.

fingolfin

This fails the manual tests:

########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/grppcext\
.gd:187 (/home/travis/build/gap-system/gap/tst/testmanuals/chapter46.tst:153)
# Input is:
D := DirectProduct( A, B );
# Expected output:
<group of size 6 with 4 generators>
# But found:
<group with 6 generators>
########
chapter46.tst
msecs: 910

codecov · 2017-10-26T15:48:18Z

Codecov Report

Merging #1809 into master will decrease coverage by 0.01%.
The diff coverage is 41.3%.

@@            Coverage Diff             @@
##           master    #1809      +/-   ##
==========================================
- Coverage   63.14%   63.12%   -0.02%     
==========================================
  Files         967      967              
  Lines      290289   290317      +28     
  Branches    12746    12721      -25     
==========================================
- Hits       183291   183273      -18     
- Misses     104200   104247      +47     
+ Partials     2798     2797       -1

Impacted Files	Coverage Δ
lib/sgpres.gi	`73.04% <0%> (ø)`	⬆️
lib/morpheus.gi	`81.2% <100%> (+0.47%)`	⬆️
lib/csetgrp.gi	`58.69% <11.11%> (-0.91%)`	⬇️
lib/grplatt.gi	`61.86% <20%> (-0.07%)`	⬇️
lib/grpfp.gi	`63.91% <53.84%> (-0.03%)`	⬇️
lib/csetperm.gi	`87.65% <0%> (-3.71%)`	⬇️
src/permutat.c	`74.57% <0%> (-1.82%)`	⬇️
lib/ghom.gi	`72.32% <0%> (-0.68%)`	⬇️
lib/grppcint.gi	`98.64% <0%> (-0.68%)`	⬇️
src/iostream.c	`46.24% <0%> (-0.4%)`	⬇️
... and 11 more

hulpke · 2017-10-26T21:31:18Z

@fingolfin How can I see which part of the manual test fails? The travis transcript is somewhat useless.
(I thought I had fixed the problem by editing the manual example, but that does not seem to be the case.)

olexandr-konovalov · 2017-10-26T22:18:16Z

@hulpke Search for diff on that page:

########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/gprd.gd:
188 (/home/travis/build/gap-system/gap/tst/testmanuals/chapter49.tst:24)
# Input is:
p:=SemidirectProduct(au,n);
# Expected output:
<permutation group with 5 generators>
# But found:
<permutation group with 4 generators>
########
########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/gprd.gd:
188 (/home/travis/build/gap-system/gap/tst/testmanuals/chapter49.tst:39)
# Input is:
au:=AutomorphismGroup(n);
# Expected output:
<group of size 6 with 2 generators>
# But found:
<group with 4 generators>
########
########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/gprd.gd:
188 (/home/travis/build/gap-system/gap/tst/testmanuals/chapter49.tst:41)
# Input is:
apc:=IsomorphismPcGroup(au);
# Expected output:
CompositionMapping( Pcgs([ (2,3), (1,2,3) ]) -> 
[ f1, f2 ], <action isomorphism> )
# But found:
CompositionMapping( Pcgs([ (2,3), (1,3,2) ]) -> [ f1, f2 ],
 <action isomorphism> )
########
########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/gprd.gd:
188 (/home/travis/build/gap-system/gap/tst/testmanuals/chapter49.tst:46)
# Input is:
apci:=InverseGeneralMapping(apc);
# Expected output:
[ f1*f2^2, f1*f2 ] -> [ Pcgs([ f1, f2 ]) -> [ f1*f2, f2 ], 
  Pcgs([ f1, f2 ]) -> [ f2, f1 ] ]
# But found:
[ f1*f2^2, f1*f2^2, f1*f2, <identity> of ... ] -> 
[ [ f1, f2 ] -> [ f2, f1 ], [ f1, f2 ] -> [ f2, f1 ], 
  [ f1, f2 ] -> [ f1*f2, f2 ], [ f1, f2 ] -> [ f1, f2 ] ]
########
chapter49.tst

markuspf · 2017-10-26T22:18:50Z

@hulpke isn't https://travis-ci.org/gap-system/gap/jobs/293220627#L1851 what you want? (I admit due to the excessive output this is hard to find, this probably needs to be improved. We should also make it easier to run this exact test on your own computer

hulpke · 2017-10-27T15:29:50Z

@alex-konovalov @markuspf
Many thanks for your help. I had scrolled through the Travis log and failed to spot the culprit. Searching for `Diff' indeed did the trick, thanks!

fingolfin

Added two remarks / questions.

fingolfin · 2017-10-31T21:46:56Z

lib/morpheus.gi

+##
+InstallMethod(AutomorphismGroup,"test abelian",true,[IsGroup and IsFinite],
+  SIZE_FLAGS(WITH_HIDDEN_IMPS_FLAGS(FLAGS_FILTER(
+    IsSolvableGroup and IsFinite))),


I don't understand this, what's its purpose? Note that for me, it returns the value 50 -- t same as RankFilter(IsSolvableGroup and IsFinite).

@fingolfin
At some time in the past SIZE_FLAGS(WITH_HIDDEN_IMPS_FLAGS(FLAGS_FILTER was the required construction to get the value of a filter. It still persists in a few places in the library.
If this ugly constrcut has been superseded by RankFilter all the better and I'd be happy to change it (in all places).

It has - sometimes in the 90ies, in fact ;-)

You are right, thought, several SIZE_FLAGS(WITH_HIDDEN_IMPS_FLAGS(FLAGS_FILTER(foo))) constructs survive, a few of them apparently were left to allow the (now removed) "completion files" to work. I'll make a PR to get rid of as many as I can.

Done, see PR #1840

It has changed in the 90s? Next you tell me that Helmut Kohl is not chancellor any longer!

I've changed it as well. Do I now wait for you to clear the review, or do I dismiss it?

fingolfin · 2017-10-31T21:48:20Z

lib/grplatt.gi

@@ -2349,7 +2349,7 @@ local uind,subs,incl,i,j,k,m,gens,t,c,p,conj,bas,basl,r;
    TryNextMethod();
  fi;
  uind:=IndexNC(G,U);
-  if uind<200 then
+  if uind<200 and ValueOption("usemaximals")<>true then


This line is added by a commit with the commit message "Added Synonym: EmbeddedConjugates, EmbeddingConjugates". Is this change here intentional? If so, it probably should be in another commit...

They are one commit as both minor additions to the intermediate subgroups code. I'll change the commit text.

fingolfin · 2017-11-01T10:16:44Z

The test failure is spurious and caused by a regression sneaking into master, and could be resolved by rebasing this branch.

I have two minor questions on this PR (see code comments above), otherwise it seems fine to merge.

fingolfin · 2017-11-02T12:52:49Z

This could be merged from my POV. @hulpke OK?

olexandr-konovalov · 2017-11-03T00:59:34Z

I have set up new Jenkins build which is parametrised by the number of pull request. At the moment, it will test a 64-bit build on Linux, running all six targets:

make test{install,standard,bugfix,manuals,packages,packagesload}

It just started to test this PR, so we will know the outcome by GMT morning.

In the future, this Jenkins build may be used for final checks of some critical PRs before merging them. This will allow us to run tests without and with all packages, and to run time-consuming tests that we currently omit on Travis, for extra checks.

olexandr-konovalov

In extended tests, no errors detected while testing
without packages. But with all packages loaded we have:

########> 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/teststandard/reesmat.tst:693
# Input is:
ForAll(enum, x-> x in U);
# Expected output:
true
# But found:
false
########
########> 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/teststandard/reesmat.tst:712
# Input is:
ForAll(enum, x-> x in UU);
# Expected output:
true
# But found:
false
########

I will investigate further.

olexandr-konovalov

This interacts with Semigroups in some unwanted way:

$ gapdev -r -A
 ┌───────┐   GAP 4.8.8-4506-g2636dd0 of today
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-apple-darwin15.6.0-gcc-6-default64
 Configuration:  gmp 6.1.2, readline
 Loading the library and packages ...
 Packages:   GAPDoc 1.6, PrimGrp 3.1.2, smallgrp 1.2, transgrp 2.0
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> LoadPackage("genss");
──────────────────────────────────────────────────────────────────────────────────────────────────────
Loading  IO 4.4.6 (Bindings for low level C library I/O routines)
by Max Neunhöffer (http://www-groups.mcs.st-and.ac.uk/~neunhoef).
Homepage: https://gap-packages.github.io/io
──────────────────────────────────────────────────────────────────────────────────────────────────────
──────────────────────────────────────────────────────────────────────────────────────────────────────
Loading  orb 4.7.6 (Methods to enumerate orbits)
by Juergen Mueller (http://www.math.rwth-aachen.de/~Juergen.Mueller),
   Max Neunhöffer (http://www-groups.mcs.st-and.ac.uk/~neunhoef), and
   Felix Noeske (http://www.math.rwth-aachen.de/~Felix.Noeske).
Homepage: https://gap-packages.github.io/orb
──────────────────────────────────────────────────────────────────────────────────────────────────────
──────────────────────────────────────────────────────────────────────────────────────────────────────
Loading  genss 1.6.4 (Generic Schreier-Sims)
by Max Neunhöffer (http://www-groups.mcs.st-and.ac.uk/~neunhoef) and
   Felix Noeske (http://www.math.rwth-aachen.de/~Felix.Noeske).
Homepage: https://gap-packages.github.io/genss
──────────────────────────────────────────────────────────────────────────────────────────────────────
true
gap> LoadPackage("dig");
──────────────────────────────────────────────────────────────────────────────────────────────────────
Loading  GRAPE 4.7 (GRaph Algorithms using PErmutation groups)
by Leonard H. Soicher (http://www.maths.qmul.ac.uk/~leonard/).
Homepage: http://www.maths.qmul.ac.uk/~leonard/grape/
──────────────────────────────────────────────────────────────────────────────────────────────────────
──────────────────────────────────────────────────────────────────────────────────────────────────────
Loading  Digraphs 0.10.1 (Digraphs - Methods for digraphs)
by J. De Beule (http://homepages.vub.ac.be/~jdbeule/),
   J. Jonusas (http://www-groups.mcs.st-andrews.ac.uk/~julius/),
   J. D. Mitchell (http://goo.gl/ZtViV6),
   M. Torpey (http://www-groups.mcs.st-andrews.ac.uk/~mct25/), and
   W. A. Wilson (http://www-groups.mcs.st-andrews.ac.uk/~waw7/).
Homepage: https://gap-packages.github.io/Digraphs
──────────────────────────────────────────────────────────────────────────────────────────────────────
true
gap> Test("reesmat.tst");
reesmat.tst                         
msecs: 33571
true
gap> LoadPackage("semi");
-----------------------------------------------------------------------------
Loading  Semigroups 3.0.7
by J. D. Mitchell (http://www-groups.mcs.st-andrews.ac.uk/~jamesm/)
with contributions by:
     S. Burrell (http://sburrell.nfshost.com),
     M. Delgado (http://cmup.fc.up.pt/cmup/mdelgado/),
     J. East (http://goo.gl/MuiJu5),
     A. Egri-Nagy (http://www.egri-nagy.hu),
     N. Ham (https://n-ham.github.io),
     J. Jonusas (http://www-groups.mcs.st-andrews.ac.uk/~julius/),
     M. Pfeiffer (https://www.morphism.de/~markusp/),
     C. Russell,
     B. Steinberg (http://www.sci.ccny.cuny.edu/~benjamin/),
     J. Smith (http://math.sci.ccny.cuny.edu/people?name=Jhevon_Smith),
     M. Torpey (http://www-groups.mcs.st-and.ac.uk/~mct25/),
 and W. A. Wilson (http://wilf.me).
-----------------------------------------------------------------------------
true
gap> Test("reesmat.tst");
########> Diff in reesmat.tst:693   
# Input is:
ForAll(enum, x-> x in U);
# Expected output:
true
# But found:
false
########
########> Diff in reesmat.tst:712
# Input is:
ForAll(enum, x-> x in UU);
# Expected output:
true
# But found:
false
########
reesmat.tst
msecs: 1439
false
gap>

@james-d-mitchell do you have an idea?

markuspf · 2017-11-03T12:54:58Z

@james-d-mitchell is this maybe related to the breaking of hash functions for permutations in "orb" (and hence has nothing to do with @hulpke's changes).

olexandr-konovalov · 2017-11-03T13:05:12Z

@markuspf thanks for pointing out that. If @james-d-mitchell confirms, then I will happily merge this.

hulpke · 2017-11-03T20:56:25Z

@fingolfin Yes, I was just planning to merge and just saw @alex-konovalov 's hold

@alex-konovalov
I have no plausible idea how any of these changes could affect semigroups (unless the semigroups code has bugs that only show up in some situations). As always, I'm happy to investigate if someone can point to a problem that is clearly in the library code.

olexandr-konovalov · 2017-11-03T23:54:08Z

@hulpke OK, I've checked this and now happy to merge it (should it be merged or rebased???). The reesmat.tst problem is reproducible without this PR too.

Use RankFilter.

even if `Image' of homomorphisms wants to use differently. This fixes gap-system#1774

Added Synonym: EmbeddedConjugates, EmbeddingConjugates also added private option to ease debugging by forcing old code to run.

when comparing double cosets.

Otherwise manual tests fail due to changes in automorphism group generators.

further names of components: "one" and "useAddition". This fixes gap-system#1819.

even if NoPrecomputedData option is set -- otherwise the calculation cannot but fail. This fixes gap-system#1822.

olexandr-konovalov · 2018-01-29T22:43:19Z

@hulpke added to release notes:

Fix an error computing kernel of group homomorphism from fp group into permutation group
Fix an error in MTC losing components when copying a new augmented coset table

If there is more to add, please let me know.

hulpke mentioned this pull request Oct 24, 2017

Error computing kernel of group hom from fp group into perm group #1774

Closed

fingolfin requested changes Oct 26, 2017

View reviewed changes

hulpke force-pushed the additions branch from 7036e1b to e9b450a Compare October 27, 2017 15:23

ChrisJefferson approved these changes Oct 31, 2017

View reviewed changes

fingolfin reviewed Oct 31, 2017

View reviewed changes

hulpke force-pushed the additions branch from e9b450a to 82c8d91 Compare October 31, 2017 22:15

olexandr-konovalov mentioned this pull request Nov 1, 2017

Cannot get perfect subgroups without data library #1822

Closed

hulpke force-pushed the additions branch 2 times, most recently from a4933f1 to 2636dd0 Compare November 1, 2017 20:31

fingolfin approved these changes Nov 1, 2017

View reviewed changes

olexandr-konovalov added this to the GAP 4.9.0 milestone Nov 3, 2017

olexandr-konovalov reviewed Nov 3, 2017

View reviewed changes

olexandr-konovalov requested changes Nov 3, 2017

View reviewed changes

olexandr-konovalov approved these changes Nov 3, 2017

View reviewed changes

hulpke added 4 commits November 5, 2017 10:51

ENHANCE: Force abelian test before computing automorphism group.

e6f056c

Use RankFilter.

Fix: Pseudorandom for fp on rank 1.

3785333

FIX: Ensure same generating set is used in `Intersection'

8c349f1

even if `Image' of homomorphisms wants to use differently. This fixes gap-system#1774

ENHANCE: Minor improvements to intermediate subgroups

43e116f

Added Synonym: EmbeddedConjugates, EmbeddingConjugates also added private option to ease debugging by forcing old code to run.

hulpke added 4 commits November 5, 2017 10:51

ENHANCE: Robustness fix for DoConjugateInto, less storage

f576c97

when comparing double cosets.

Change examples in manual to not require number of generators.

6b5def8

Otherwise manual tests fail due to changes in automorphism group generators.

FIX: When copying a new augmented coset table also allow for

1252eda

further names of components: "one" and "useAddition". This fixes gap-system#1819.

ENHANCE: Allow perfect subgroups from library of perfect groups

0cb7461

even if NoPrecomputedData option is set -- otherwise the calculation cannot but fail. This fixes gap-system#1822.

hulpke force-pushed the additions branch from 2636dd0 to 0cb7461 Compare November 5, 2017 17:52

hulpke merged commit 67dc403 into gap-system:master Nov 5, 2017

olexandr-konovalov mentioned this pull request Nov 6, 2017

Problem with reesmat.tst (orb+semigroups) #1860

Closed

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

Fixes/Improvements that should make the 4.9 release #1809

Fixes/Improvements that should make the 4.9 release #1809

hulpke commented Oct 24, 2017 •

edited by olexandr-konovalov

fingolfin left a comment

codecov bot commented Oct 26, 2017 •

edited

hulpke commented Oct 26, 2017

olexandr-konovalov commented Oct 26, 2017

markuspf commented Oct 26, 2017

hulpke commented Oct 27, 2017

fingolfin left a comment

fingolfin Oct 31, 2017

hulpke Nov 1, 2017 •

edited

fingolfin Nov 1, 2017

fingolfin Nov 1, 2017

hulpke Nov 1, 2017

fingolfin Oct 31, 2017

hulpke Nov 1, 2017

fingolfin Nov 1, 2017

fingolfin commented Nov 1, 2017

fingolfin commented Nov 2, 2017

olexandr-konovalov commented Nov 3, 2017

olexandr-konovalov left a comment

olexandr-konovalov left a comment

markuspf commented Nov 3, 2017

olexandr-konovalov commented Nov 3, 2017

hulpke commented Nov 3, 2017 •

edited

olexandr-konovalov commented Nov 3, 2017 •

edited

olexandr-konovalov commented Jan 29, 2018

Fixes/Improvements that should make the 4.9 release #1809

Fixes/Improvements that should make the 4.9 release #1809

Conversation

hulpke commented Oct 24, 2017 • edited by olexandr-konovalov

fingolfin left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 26, 2017 • edited

Codecov Report

hulpke commented Oct 26, 2017

olexandr-konovalov commented Oct 26, 2017

markuspf commented Oct 26, 2017

hulpke commented Oct 27, 2017

fingolfin left a comment

Choose a reason for hiding this comment

fingolfin Oct 31, 2017

Choose a reason for hiding this comment

hulpke Nov 1, 2017 • edited

Choose a reason for hiding this comment

fingolfin Nov 1, 2017

Choose a reason for hiding this comment

fingolfin Nov 1, 2017

Choose a reason for hiding this comment

hulpke Nov 1, 2017

Choose a reason for hiding this comment

fingolfin Oct 31, 2017

Choose a reason for hiding this comment

hulpke Nov 1, 2017

Choose a reason for hiding this comment

fingolfin Nov 1, 2017

Choose a reason for hiding this comment

fingolfin commented Nov 1, 2017

fingolfin commented Nov 2, 2017

olexandr-konovalov commented Nov 3, 2017

olexandr-konovalov left a comment

Choose a reason for hiding this comment

olexandr-konovalov left a comment

Choose a reason for hiding this comment

markuspf commented Nov 3, 2017

olexandr-konovalov commented Nov 3, 2017

hulpke commented Nov 3, 2017 • edited

olexandr-konovalov commented Nov 3, 2017 • edited

olexandr-konovalov commented Jan 29, 2018

hulpke commented Oct 24, 2017 •

edited by olexandr-konovalov

codecov bot commented Oct 26, 2017 •

edited

hulpke Nov 1, 2017 •

edited

hulpke commented Nov 3, 2017 •

edited

olexandr-konovalov commented Nov 3, 2017 •

edited