Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Invalid result of ConjugacyClassesSubgroups(TransitiveGroup(6,14)); if "tomlib" and "atlasrep" are loaded #2586

Closed
fingolfin opened this issue Jun 28, 2018 · 6 comments
Closed

Invalid result of ConjugacyClassesSubgroups(TransitiveGroup(6,14)); if "tomlib" and "atlasrep" are loaded #2586

fingolfin opened this issue Jun 28, 2018 · 6 comments
Assignees
@hulpke
Labels
kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them regression A bug that only occurs in the branch, not in a release
Milestone
GAP 4.9.2

Comments

@fingolfin
Copy link
Member

I stumbled over the following regression in master by chance. It was introduced by 3608af5 by @hulpke with commit message

ENHANCE: Use TOM data also for subgroups of almost simple

Also avoid pc isom in NormalizerParemtSA if the task is pathetic anyhow.

This is a regression compared to GAP 4.9.

Here is an example of the bad output. Note that (1,2) is not even contained in TransitiveGroup(6,14)

$ gap -A
 ┌───────┐   GAP 4.10dev-844-g6347a70-dirty of today
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-apple-darwin15.6.0-default64
 Configuration:  gmp 6.1.2, readline
 Loading the library and packages ...
 Packages:   GAPDoc 1.6.1.dev, PrimGrp 3.3.1, SmallGrp 1.3, TransGrp 2.0.2
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> ConjugacyClassesSubgroups(TransitiveGroup(6,14));
[ Group( () )^G, Group( [ (1,2)(3,4)(5,6) ] )^G, Group( [ (3,6)(4,5) ] )^G,
  Group( [ (1,2,3)(4,5,6) ] )^G, Group( [ (3,6)(4,5), (1,2)(4,5) ] )^G,
  Group( [ (3,6)(4,5), (3,4,6,5) ] )^G,
  Group( [ (3,6)(4,5), (1,2)(3,4)(5,6) ] )^G, Group( [ (2,3,5,4,6) ] )^G,
  Group( [ (1,2)(3,4)(5,6), (1,4,5)(2,3,6) ] )^G,
  Group( [ (1,2,3)(4,5,6), (2,3)(5,6) ] )^G,
  Group( [ (1,2,3)(4,5,6), (1,4)(2,6)(3,5) ] )^G,
  Group( [ (3,6)(4,5), (3,4,6,5), (1,2)(4,5) ] )^G,
  Group( [ (2,3,5,4,6), (3,6)(4,5) ] )^G,
  Group( [ (3,6)(4,5), (1,2)(4,5), (1,3,4)(2,6,5) ] )^G,
  Group( [ (1,2)(3,4)(5,6), (1,4,5)(2,3,6), (3,6)(4,5) ] )^G,
  Group( [ (2,3,5,4,6), (3,6)(4,5), (3,4,6,5) ] )^G,
  Group( [ (3,6)(4,5), (1,2)(4,5), (1,3,4)(2,6,5), (3,4,6,5) ] )^G,
  Group( [ (1,3,6)(2,5,4), (1,2,4)(3,5,6), (1,4,5)(2,3,6) ] )^G,
  L(6):2 = PGL(2,5) = S_5(6)^G ]
gap> LoadPackage("tomlib":OnlyNeeded);
─────────────────────────────────────────────────────────────────────────────
Loading  AtlasRep 1.5.1 (An Atlas of Group Representations)
by Robert A. Wilson (http://www.maths.qmw.ac.uk/~raw),
   Richard A. Parker (richpark@gmx.co.uk),
   Simon Nickerson (http://nickerson.org.uk/groups),
   John N. Bray (http://www.maths.qmw.ac.uk/~jnb), and
   Thomas Breuer (http://www.math.rwth-aachen.de/~Thomas.Breuer).
Homepage: http://www.math.rwth-aachen.de/~Thomas.Breuer/atlasrep
─────────────────────────────────────────────────────────────────────────────
─────────────────────────────────────────────────────────────────────────────
Loading  TomLib 1.2.6 (The GAP Library of Tables of Marks)
by Götz Pfeiffer (goetz.pfeiffer@nuigalway.ie).
Homepage: http://schmidt.nuigalway.ie/tomlib
─────────────────────────────────────────────────────────────────────────────
true
gap> ConjugacyClassesSubgroups(TransitiveGroup(6,14));
[ Group( () )^G, Group( [ (1,2) ] )^G, Group( [ (2,4)(3,5) ] )^G,
  Group( [ (1,2,5) ] )^G, Group( [ (2,5)(3,4), (2,4)(3,5) ] )^G,
  Group( [ (2,3,4,5) ] )^G, Group( [ (4,5), (2,3) ] )^G,
  Group( [ (1,3,4,5,2) ] )^G, Group( [ (4,5), (3,4,5) ] )^G,
  Group( [ (1,2)(3,4), (3,4,5) ] )^G, Group( [ (1,3,5)(2,4) ] )^G,
  Group( [ (2,3), (2,4)(3,5) ] )^G, Group( [ (2,5)(3,4), (1,2,3,4,5) ] )^G,
  Group( [ (3,4,5), (2,4)(3,5) ] )^G, Group( [ (4,5), (1,2,3), (1,2) ] )^G,
  Group( [ (1,2)(3,4), (2,3,4,5) ] )^G, Group( [ (3,5,4), (2,3,4,5) ] )^G,
  Group( [ (2,4)(3,5), (1,2,5) ] )^G, SymmetricGroup( [ 1 .. 5 ] )^G ]
gap> (1,2) in TransitiveGroup(6,14);
false
gap>
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them regression A bug that only occurs in the branch, not in a release labels Jun 28, 2018
@fingolfin
Copy link
Member Author

For the record, I found this by running Sanity() from tst/sanity.g. Unfortunately, this test runs for a very long time, hence we don't run this automatically right now... But perhaps we can run it at least occasionally, or on Jenkins? Perhaps @alex-konovalov has some ideas on this?

I also found a second problem, which traces back to the same commit: Sometimes, this error happens (but only if tomlib is loaded):

gap> ConjugacyClassesSubgroups(SmallGroup(120,5));
Error, usage: PreImage(<map>), PreImage(<map>,<img>), PreImage(<map>,<coll>) at /Users/mhorn/Projekte/GAP/gap.github/lib/mapping.gi:302 called from
PreImage( hom, a ) at /Users/mhorn/Projekte/GAP/gap.github/lib/grplatt.gi:945 called from
LatticeSubgroups( G ) at /Users/mhorn/Projekte/GAP/gap.github/lib/grplatt.gi:208 called from
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:1
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk>

@hulpke
Copy link
Contributor

hulpke commented Jun 28, 2018

@fingolfin
Yes, sanity is a terrific tests that finds all kinds of silly errors. I have a fix for the first bug. (Added for #2488 which is still being held for missing review.)

I cannot reproduce the second error. If you can, what is hom and what is a? (Is it broken objects or wrong domain/image of the map?)

hulpke added a commit to hulpke/gap that referenced this issue Jun 28, 2018
@hulpke
FIX: Pull subgroups of TOM in lattice through isomorphism back.
50ef5c5 
This silly bug likely did not show up earlier since the TOM library was not
loaded by default.

This partially fixes gap-system#2586
(main bug reported there)
hulpke added a commit to hulpke/gap that referenced this issue Jun 28, 2018
@hulpke
FIX: Pull subgroups of TOM for lattice back through isomorphism
6780793 
This bug likely did not show up earlier since the TOM library was not loaded
by default.

This partially fixes gap-system#2586
(main bug reported there)
@hulpke hulpke mentioned this issue Jun 28, 2018
Merged
@fingolfin
Copy link
Member Author

@hulpke Thanks for the quick fix of the first issue. Here is a log with the information you asked for:

$ ./gap -A
 ┌───────┐   GAP 4.10dev-843-g6f7c0a0 of today
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-apple-darwin15.6.0-default64
 Configuration:  gmp 6.1.2, readline
 Loading the library and packages ...
 Packages:   GAPDoc 1.6.1, PrimGrp 3.3.0, smallgrp 1.2, TransGrp 2.0.2
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> ConjugacyClassesSubgroups(SmallGroup(120,5));
[ Group( () )^G, Group( [ ( 1, 4)( 2, 8)( 3, 9)( 5, 6)( 7,13)(10,11)(12,17)(14,15)(16,21)(18,19)(20,24)(22,23)
     ] )^G, Group( [ ( 1, 3, 7)( 2, 5,10)( 4, 9,13)( 6,11, 8)(12,16,20)(14,18,22)(15,19,23)(17,21,24) ] )^G,
  Group( [ ( 1, 8, 4, 2)( 3, 5, 9, 6)( 7,17,13,12)(10,15,11,14)(16,24,21,20)(18,23,19,22) ] )^G,
  Group( [ ( 3,13,23,21,15)( 5,11,20,19,12)( 6,10,24,18,17)( 7,22,16,14, 9) ] )^G,
  Group( [ ( 1, 3, 7)( 2, 5,10)( 4, 9,13)( 6,11, 8)(12,16,20)(14,18,22)(15,19,23)(17,21,24),
      ( 1, 4)( 2, 8)( 3, 9)( 5, 6)( 7,13)(10,11)(12,17)(14,15)(16,21)(18,19)(20,24)(22,23) ] )^G,
  Group( [ ( 1, 8, 4, 2)( 3, 5, 9, 6)( 7,17,13,12)(10,15,11,14)(16,24,21,20)(18,23,19,22),
      ( 1, 6, 4, 5)( 2, 3, 8, 9)( 7,11,13,10)(12,14,17,15)(16,22,21,23)(18,24,19,20) ] )^G,
  Group( [ ( 3,13,23,21,15)( 5,11,20,19,12)( 6,10,24,18,17)( 7,22,16,14, 9), ( 1, 4)( 2, 8)( 3, 9)( 5, 6)( 7,13)
        (10,11)(12,17)(14,15)(16,21)(18,19)(20,24)(22,23) ] )^G,
  Group( [ ( 1, 3, 7)( 2, 5,10)( 4, 9,13)( 6,11, 8)(12,16,20)(14,18,22)(15,19,23)(17,21,24),
      ( 1, 8, 4, 2)( 3,11, 9,10)( 5, 7, 6,13)(12,22,17,23)(14,24,15,20)(16,18,21,19) ] )^G,
  Group( [ ( 3,13,23,21,15)( 5,11,20,19,12)( 6,10,24,18,17)( 7,22,16,14, 9),
      ( 1, 8, 4, 2)( 3, 5, 9, 6)( 7,17,13,12)(10,15,11,14)(16,24,21,20)(18,23,19,22) ] )^G,
  Group( [ ( 1, 8, 4, 2)( 3, 5, 9, 6)( 7,17,13,12)(10,15,11,14)(16,24,21,20)(18,23,19,22),
      ( 1, 6, 4, 5)( 2, 3, 8, 9)( 7,11,13,10)(12,14,17,15)(16,22,21,23)(18,24,19,20),
      ( 1,11,18)( 2, 7,21)( 3,15,20)( 4,10,19)( 5,12,22)( 6,17,23)( 8,13,16)( 9,14,24) ] )^G,
  Group( [ ( 1, 3, 7)( 2, 5,10)( 4, 9,13)( 6,11, 8)(12,16,20)(14,18,22)(15,19,23)(17,21,24),
      ( 1, 8, 4, 2)( 3, 5, 9, 6)( 7,17,13,12)(10,15,11,14)(16,24,21,20)(18,23,19,22) ] )^G ]
gap> LoadPackage("tomlib":OnlyNeeded);
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Loading  AtlasRep 1.5.1 (An Atlas of Group Representations)
by Robert A. Wilson (http://www.maths.qmw.ac.uk/~raw),
   Richard A. Parker (richpark@gmx.co.uk),
   Simon Nickerson (http://nickerson.org.uk/groups),
   John N. Bray (http://www.maths.qmw.ac.uk/~jnb), and
   Thomas Breuer (http://www.math.rwth-aachen.de/~Thomas.Breuer).
Homepage: http://www.math.rwth-aachen.de/~Thomas.Breuer/atlasrep
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Loading  TomLib 1.2.6 (The GAP Library of Tables of Marks)
by Götz Pfeiffer (goetz.pfeiffer@nuigalway.ie).
Homepage: http://schmidt.nuigalway.ie/tomlib
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────
true
gap> ConjugacyClassesSubgroups(SmallGroup(120,5));
Error, usage: PreImage(<map>), PreImage(<map>,<img>), PreImage(<map>,<coll>) at /Users/mhorn/Projekte/GAP/gap.github/build/default/../../lib/mapping.gi:302 called from
PreImage( hom, a ) at /Users/mhorn/Projekte/GAP/gap.github/build/default/../../lib/grplatt.gi:945 called from
LatticeSubgroups( G ) at /Users/mhorn/Projekte/GAP/gap.github/build/default/../../lib/grplatt.gi:208 called from
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:3
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> hom;
[ (1,3,7)(2,5,10)(4,9,13)(6,11,8)(12,16,20)(14,18,22)(15,19,23)(17,21,24), (1,2,4,8)(3,6,9,5)(7,12,13,17)(10,14,
    11,15)(16,20,21,24)(18,22,19,23), (1,4)(2,8)(3,9)(5,6)(7,13)(10,11)(12,17)(14,15)(16,21)(18,19)(20,24)(22,23)
 ] -> [ (1,2,3)(4,5,6), (3,4)(5,6), () ]
brk> a;
Group([ (2,3)(4,5) ])
brk> Range(hom);
Group([ (1,2,3)(4,5,6), (3,4)(5,6), () ])
brk> a in Range(hom);
false

@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.2 milestone Jun 28, 2018
@olexandr-konovalov
Copy link
Member

@fingolfin good catch! About running this more often - just how long is very long for sanity.g? Are we speaking of hours or days?

I run tests from benchmark weekly on Jenkins. At the moment all pass in master branch, and grpauto runs out of memory in stable-4.9 branch (known problem, resolved in master). It wouldn't be a problem to have another benchmark based on sanity.g, also checking if it may have some overlaps with other benchmarks also doing some sanity checks already).

hulpke added a commit to hulpke/gap that referenced this issue Jun 29, 2018
@hulpke
FIX: Pull subgroups of TOM for lattice back through isomorphism
88969f6 
This bug likely did not show up earlier since the TOM library was not loaded
by default.

Also same fix in second place where code duplicates

Even an example is provided, the latter thrice as error does not arise every
time.

This fixes gap-system#2586
@hulpke
Copy link
Contributor

hulpke commented Jun 29, 2018

@fingolfin
Thanks for the details. No surprise I had not been able to reproduce it -- it was basically the same bug.
Is fixed now.

hulpke added a commit to hulpke/gap that referenced this issue Jun 29, 2018
@hulpke
FIX: Pull subgroups of TOM through lattice
734881e 
This bug likely did not show up earlier since the TOM library was not loaded
by default.

This partially fixes gap-system#2586
(main bug reported there)
hulpke added a commit to hulpke/gap that referenced this issue Jun 29, 2018
@hulpke
FIX: Pull subgroups of TOM for lattice back through isomorphism
dc2c83d 
This bug likely did not show up earlier since the TOM library was not loaded
by default.

Even an example is provided, the latter thrice as error does not arise every
time.

This fixes gap-system#2586
@hulpke
Copy link
Contributor

hulpke commented Jun 29, 2018

@alex-konovalov
Based on my experience of 18 years ago, I think running sanity.g at least once at the start of each release process is extremely useful. For regular schedule I'd suggest once every 4-8 weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hulpke hulpke

Labels
kind: bug: wrong result Issues describing bugs that result in mathematically or otherwise wrong results, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them regression A bug that only occurs in the branch, not in a release
Projects
None yet
Milestone
GAP 4.9.2
Development

No branches or pull requests
3 participants
@fingolfin @olexandr-konovalov @hulpke