gap-packages / wedderga

Wedderburn Decomposition of Group Algebras
https://gap-packages.github.io/wedderga/
GNU General Public License v2.0
3 stars 6 forks source link

Add test for large algebras with four generated Galois groups #76

Closed angeldelriomateos closed 3 years ago

angeldelriomateos commented 4 years ago

Ok, I am going to modify it taking your comments into account and commit it again.

codecov[bot] commented 4 years ago

Codecov Report

Merging #76 into divalg will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           divalg      #76   +/-   ##
=======================================
  Coverage   81.59%   81.59%           
=======================================
  Files          11       11           
  Lines        5193     5193           
=======================================
  Hits         4237     4237           
  Misses        956      956           
olexandr-konovalov commented 4 years ago

I have tested only groups 236000, 236001, 236002, 236003 - test finally passed within time limits of Travis. The test does not change coverage at all though! I will now swap the test to test the other three groups instead.

olexandr-konovalov commented 4 years ago

The same is for the other three groups: 236007, 236008, 236009. Both, of course, are with WedderburnDecompositionInfo calls commented out. I will try again with them enabled.

olexandr-konovalov commented 4 years ago

Full test for SmallGroup(1920,236000) - no coverage increase. Travis run ~12 minutes in total.

olexandr-konovalov commented 4 years ago

Same for SmallGroup(1920,236001).

olexandr-konovalov commented 4 years ago

No coverage changes for SmallGroup(1920,236002) either, runtime of the full test is a bit above 11 minutes (but only 6 minutes in stable-4.9??? https://travis-ci.org/github/gap-packages/wedderga/builds/680089711)

drallenherman commented 4 years ago

No coverage changes for SmallGroup(1920,236002) either, runtime of the full test is a bit above 11 minutes (but only 6 minutes in stable-4.9??? https://travis-ci.org/github/gap-packages/wedderga/builds/680089711)

Hi Alexander. I think the reason the groups SG(1920,236000 to 236009) are not increasing code coverage is because the RCA code cannot find any intermediate cyclotomic extensions between CF(120) and the center of the simple component. These are a good test of KillingCocycle but every check in ReducingCyclotomicAlgebra fails. Try SG(1920,136971). This one has a split simple component of dimension 16^2, which means many checks succeeded.

olexandr-konovalov commented 4 years ago

Full test for SmallGroup(1920,236003) - no coverage increase. Travis run ~12 minutes in total.

drallenherman commented 4 years ago

Full test for SmallGroup(1920,236003) - no coverage increase. Travis run ~12 minutes in total.

SG(1920,204811) also has a split component of dim 16^2. With a different center. I'm not sure how much coverage can increase. A lot of the smaller examples are already covering most of the code in the GlobalSplitting functions.

olexandr-konovalov commented 4 years ago

@drallenherman do you mean such test:

gap> QG:=GroupRing(Rationals,SmallGroup(1920,136971));;
gap> W:=WedderburnDecompositionInfo(QG);;time;
7150
gap> A:=W[Length(W)];
[ 16, GaussianRationals ]
gap> LocalIndicesOfCyclotomicAlgebra(A);time;
[  ]
0
gap> QG:=GroupRing(Rationals,SmallGroup(1920,204811));;
gap> W:=WedderburnDecompositionInfo(QG);;time;
11181
gap> A:=W[Length(W)];
[ 16, NF(20,[ 1, 3, 7, 9 ]) ]
gap> LocalIndicesOfCyclotomicAlgebra(A);time;
[  ]
1

if so, I will add it them div-alg.tst, the runtime is affordable.

drallenherman commented 4 years ago

@drallenherman do you mean such test:

gap> QG:=GroupRing(Rationals,SmallGroup(1920,136971));;
gap> W:=WedderburnDecompositionInfo(QG);;time;
7150
gap> A:=W[Length(W)];
[ 16, GaussianRationals ]
gap> LocalIndicesOfCyclotomicAlgebra(A);time;
[  ]
0
gap> QG:=GroupRing(Rationals,SmallGroup(1920,204811));;
gap> W:=WedderburnDecompositionInfo(QG);;time;
11181
gap> A:=W[Length(W)];
[ 16, NF(20,[ 1, 3, 7, 9 ]) ]
gap> LocalIndicesOfCyclotomicAlgebra(A);time;
[  ]
1

if so, I will add it them div-alg.tst, the runtime is affordable.

Yes. No promises, though, these might also do nothing to improve coverage.

olexandr-konovalov commented 4 years ago

So, none of these examples produce extra coverage, and each single group takes more than 10 minutes - this is is too much for a standard test. We recommend the whole duration of a testall.g to be no longer than 10, at most 15, minutes.

If you think that this is useful to keep as an extended test, I will update this PR to move it to a testextra directory where we will be able to run it manually, e.g. to check for memory and runtime regressions. Should I?

drallenherman commented 4 years ago

So, none of these examples produce extra coverage, and each single group takes more than 10 minutes - this is is too much for a standard test. We recommend the whole duration of a testall.g to be no longer than 10, at most 15, minutes.

If you think that this is useful to keep as an extended test, I will update this PR to move it to a testextra directory where we will be able to run it manually, e.g. to check for memory and runtime regressions. Should I?

Sure. For this type of test it could be useful to just pick one group like #236000 and run WDInfo and LocalIndicesOfCA on all of the simple components. The other ones would run almost exactly the same calculations, with just slightly different results.

angeldelriomateos commented 4 years ago

Do we know which part of the code is not covered by the tests available so far?

angeldelriomateos commented 4 years ago

I mean, we can design a test to cover the part not covered yet.

olexandr-konovalov commented 4 years ago

@angeldelriomateos we know - please see results for the divalg branch at https://codecov.io/gh/gap-packages/wedderga/tree/divalg/lib

olexandr-konovalov commented 4 years ago

Since #58 is merged, I have changed this PR so it's now submitted to master, but it has to be rebased on master, if we are going ahead with it in some form, to get rid of all commits that are already merged.

olexandr-konovalov commented 3 years ago

@angeldelriomateos @drallenherman what are we going to do with this PR?

drallenherman commented 3 years ago

I do not think this PR is needed anymore @alex-konovalov. We gave our last version the capability to deal with Galois groups with four or more generators. But we did that by merging from a different PR.