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.

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

JenningsLieAlgebra #2079

Closed
wants to merge 4 commits into from
Closed

Conversation

willemdegraaf
Copy link
Contributor

@willemdegraaf willemdegraaf commented Jan 11, 2018

Fixed a bug in JenningsLieAlgebra, reported by Laurent Bartholdi.
Here is an example of the correct behaviour:

gap> g := Group((1,10)(2,9)(3,11)(4,12)(5,15)(6,16)(7,13)(8,14)(17,21)(18,22)*
> (19,24)(20,23)(25,27)(26,28)(29,30)(33,51)(34,52)(35,49)(36,50)(37,54)(38,53)*
> (39,55)(40,56)(41,61)(42,62)(43,64)(44,63)(45,57)(46,58)(47,60)(48,59), 
> (1,19)(2,20)(3,17)(4,18)(5,22)(6,21)(7,23)(8,24)(9,29)(10,30)(11,32)(12,31)*
> (13,25)(14,26)(15,28)(16,27)(33,42)(34,41)(35,43)(36,44)(37,47)(38,48)(39,45)*
> (40,46)(49,53)(50,54)(51,56)(52,55)(57,59)(58,60)(61,62), 
> (1,37)(2,38)(3,40)(4,39)(5,33)(6,34)(7,36)(8,35)(9,43)(10,44)(11,41)(12,42)*
> (13,46)(14,45)(15,47)(16,48)(17,58)(18,57)(19,59)(20,60)(21,63)(22,64)(23,61)*
> (24,62)(25,50)(26,49)(27,51)(28,52)(29,55)(30,56)(31,53)(32,54));
<permutation group with 3 generators>
gap> L:= JenningsLieAlgebra(g);
<Lie algebra of dimension 13 over GF(2)>
gap> List(Basis(L),PthPowerImage);
[ 0*v.1, v.6, 0*v.1, v.7, v.8, v.9, 0*v.1, v.10, v.11, v.12, 0*v.1, v.13, 
  0*v.1 ]
gap> LieLowerCentralSeries(L);
[ <Lie algebra of dimension 13 over GF(2)>, 
  <Lie algebra of dimension 3 over GF(2)>, 
  <Lie algebra of dimension 0 over GF(2)> ]

Fixed a bug reported by Laurent Bartholdi. 
Here is an example showing the correct behaviour:

gap> g := Group((1,10)(2,9)(3,11)(4,12)(5,15)(6,16)(7,13)(8,14)(17,21)(18,22)*
> (19,24)(20,23)(25,27)(26,28)(29,30)(33,51)(34,52)(35,49)(36,50)(37,54)(38,53)*
> (39,55)(40,56)(41,61)(42,62)(43,64)(44,63)(45,57)(46,58)(47,60)(48,59), 
> (1,19)(2,20)(3,17)(4,18)(5,22)(6,21)(7,23)(8,24)(9,29)(10,30)(11,32)(12,31)*
> (13,25)(14,26)(15,28)(16,27)(33,42)(34,41)(35,43)(36,44)(37,47)(38,48)(39,45)*
> (40,46)(49,53)(50,54)(51,56)(52,55)(57,59)(58,60)(61,62), 
> (1,37)(2,38)(3,40)(4,39)(5,33)(6,34)(7,36)(8,35)(9,43)(10,44)(11,41)(12,42)*
> (13,46)(14,45)(15,47)(16,48)(17,58)(18,57)(19,59)(20,60)(21,63)(22,64)(23,61)*
> (24,62)(25,50)(26,49)(27,51)(28,52)(29,55)(30,56)(31,53)(32,54));
<permutation group with 3 generators>
gap> L:= JenningsLieAlgebra(g);
<Lie algebra of dimension 13 over GF(2)>
gap> List(Basis(L),PthPowerImage);
[ 0*v.1, v.6, 0*v.1, v.7, v.8, v.9, 0*v.1, v.10, v.11, v.12, 0*v.1, v.13, 
  0*v.1 ]
gap> LieLowerCentralSeries(L);
[ <Lie algebra of dimension 13 over GF(2)>, 
  <Lie algebra of dimension 3 over GF(2)>, 
  <Lie algebra of dimension 0 over GF(2)> ]
removed a line break...
@markuspf
Copy link
Member

Without knowing what's going on, could you add a test, please? (For example testing the above example, feel free to add more ;))

@willemdegraaf
Copy link
Contributor Author

willemdegraaf commented Jan 11, 2018 via email

@olexandr-konovalov
Copy link
Member

@willemdegraaf you can add a test file to the tst/testbugfix directory - see some of the most recent test files there as an example what to put there and how to name the test files.

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Jan 11, 2018
@willemdegraaf
Copy link
Contributor Author

willemdegraaf commented Jan 11, 2018 via email

@olexandr-konovalov
Copy link
Member

@willemdegraaf are you using GitHub's web-interface, or working in a local clone of the repository with git?

@willemdegraaf
Copy link
Contributor Author

willemdegraaf commented Jan 11, 2018 via email

@olexandr-konovalov
Copy link
Member

@willemdegraaf then go to the branch corresponding to this pull request in your fork, and navigate there to the directory where you want to create the file - that is given by this link: https://github.com/willemdegraaf/gap/tree/Laurent/tst

Now there is a "create new file" button there, click on it, specify the file name, enter the content of the file, then the text of the commit message, and click on "Commit new file" button at the bottom of this page.

Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR breaks examples in the manual (see the build log at https://travis-ci.org/gap-system/gap/jobs/327599027):

########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/alglie.g\
d:1456 (/home/travis/build/gap-system/gap/tst/testmanuals/chapter64.tst:323)
# Input is:
L:= JenningsLieAlgebra( G );
# Expected output:
<Lie algebra of dimension 6 over GF(3)>
# But found:
Error, list entry 7 must lie in [ 1 .. 6 ]
########
########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/alglie.g\
d:1456 (/home/travis/build/gap-system/gap/tst/testmanuals/chapter64.tst:327)
# Input is:
PthPowerImages( Basis( L ) );
# Expected output:
[ v.6, 0*v.1, 0*v.1, 0*v.1, 0*v.1, 0*v.1 ]
# But found:
[ 0*v.1, v.2, 0*v.1, 0*v.1, 0*v.1, 0*v.1, 0*v.1, 0*v.1, 0*v.1, 0*v.1, 
  0*v.1 ]
########
########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/alglie.g\
d:1456 (/home/travis/build/gap-system/gap/tst/testmanuals/chapter64.tst:329)
# Input is:
g:= Grading( L );
# Expected output:
rec( hom_components := function( d ) ... end, max_degree := 3, 
  min_degree := 1, source := Integers )
# But found:
Error, no method found! For debugging hints type ?Recovery from NoMeth\
odFound
Error, no 2nd choice method found for `Grading' on 1 arguments
########
########> Diff in /home/travis/build/gap-system/gap/doc/ref/../../lib/alglie.g\
d:1456 (/home/travis/build/gap-system/gap/tst/testmanuals/chapter64.tst:332)
# Input is:
List( [1,2,3], g.hom_components );
# Expected output:
[ <vector space over GF(3), with 3 generators>, 
  <vector space over GF(3), with 2 generators>, 
  <vector space over GF(3), with 1 generators> ]
# But found:
Error, Record Element: <rec> must be a record (not a list (plain,rect \
table,imm))
########
chapter64.tst

(Most likely just fixing the 1st example would fix the others)

Fixed stupid additional error; and also the function PCentralLieAlgebra.
@codecov
Copy link

codecov bot commented Jan 11, 2018

Codecov Report

Merging #2079 into master will increase coverage by 0.19%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2079      +/-   ##
==========================================
+ Coverage   67.07%   67.26%   +0.19%     
==========================================
  Files         900      901       +1     
  Lines      273512   273451      -61     
==========================================
+ Hits       183447   183939     +492     
+ Misses      90065    89512     -553
Impacted Files Coverage Δ
lib/alglie.gi 68.19% <50%> (+0.02%) ⬆️
lib/grp.gi 84.54% <0%> (-1.63%) ⬇️
lib/init.g 70.5% <0%> (-0.54%) ⬇️
lib/ghomfp.gi 81.65% <0%> (-0.29%) ⬇️
lib/wordass.gi 60.07% <0%> (-0.27%) ⬇️
src/plist.c 91.22% <0%> (-0.13%) ⬇️
src/io.c 58.4% <0%> (-0.05%) ⬇️
src/gap.c 64.85% <0%> (-0.04%) ⬇️
src/cyclotom.c 94.47% <0%> (-0.01%) ⬇️
src/intobj.h 100% <0%> (ø) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2566f79...1811488. Read the comment docs.

@willemdegraaf
Copy link
Contributor Author

Fixed that. Also noted that the same error happens in PCentralLieAlgebra; fixed that as well.
Next I hope to add a test file.

test file for the fix of JenningsLieAlgebra and also for PCentralLieAlgebra.
@willemdegraaf
Copy link
Contributor Author

willemdegraaf commented Jan 11, 2018 via email

@olexandr-konovalov
Copy link
Member

@willemdegraaf you don't need another PR: because the file was added in the branch from which this PR has been submitted, this PR is now updated automatically.

When this PR will be ready, one should perhaps "squash and merge" this to have all changes in a single commit.

@olexandr-konovalov
Copy link
Member

@willemdegraaf ... hmm... your new test file will not be picked up by tests, because it is not in the right location - I've overlooked that, because your fork is our of sync, and it does not have the latest state of the tst directory, which has a separate subdirectory for bugfix tests. But first let's see how the currently running Travis CI tests will go.

Will you be able to use git on your local machine instead of the web interface? That may be more efficient in the long run.

@willemdegraaf
Copy link
Contributor Author

willemdegraaf commented Jan 11, 2018 via email

@olexandr-konovalov
Copy link
Member

@willemdegraaf will be time well invested - better than spending time on trying to use the web interface. Hopefully some of us here may help. To start with, you may try to follow https://github.com/gap-system/gap/blob/master/CONTRIBUTING.md

@willemdegraaf
Copy link
Contributor Author

willemdegraaf commented Jan 11, 2018 via email

@olexandr-konovalov
Copy link
Member

P.S. If this is a bugfix, perhaps it should be submitted to stable-4.9 branch. One could edit PR using GitHub web interface to change where it is submitted.

@fingolfin
Copy link
Member

This PR is based on 5343150 from April 2015 (!), i.e. 2.5 years ago.

I'll make a new PR with the changes in here, in a single commit and all the problems @alex-konovalov mentioned corrected.

@olexandr-konovalov
Copy link
Member

Closed as replaced by #2085.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants