search

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

Use elif in LocalIndicesOfRationalSymbolAlgebra #83

Closed olexandr-konovalov closed 4 years ago

olexandr-konovalov commented 4 years ago

This is a follow-up to #82 which optimises the code by using elif instead of multiple if, avoiding redundant checks.

codecov[bot] commented 4 years ago

Codecov Report

Merging #83 into divalg will increase coverage by 0.06%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           divalg      #83      +/-   ##
==========================================
+ Coverage   83.39%   83.45%   +0.06%     
==========================================
  Files          11       11              
  Lines        5180     5193      +13     
==========================================
+ Hits         4320     4334      +14     
+ Misses        860      859       -1
Impacted Files Coverage Δ
lib/div-alg.gi 93.12% <100.00%> (+0.11%) :arrow_up:
olexandr-konovalov commented 4 years ago

@drallenherman added more tests to ensure that diffs are 100% covered. Tried them in wedderga 4.9.5. Some of the diffs are from [ ] becoming fail, but there are some more. So, is all the new output corect?

########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:75
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,-2));
# Expected output:
[ fail, fail, fail, fail, fail, fail ]
# But found:
[ [  ], [ [ infinity, 2 ], [ 2, 2 ] ], [  ], [  ], [  ], [  ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:77
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,-1));
# Expected output:
[ fail, [ [ infinity, 2 ], [ 2, 2 ] ], fail, fail, [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [ [ infinity, 2 ], [ 2, 2 ] ], [ [ infinity, 2 ], [ 2, 2 ] ], [  ], [  ], 
  [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:79
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,2));
# Expected output:
[ fail, [  ], fail, fail, [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [  ], [  ], [  ], [  ], [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:81
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,3));
# Expected output:
[ fail, [ [ 2, 2 ], [ 3, 2 ] ], fail, fail, [ [ 2, 2 ], [ 3, 2 ] ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [  ], [ [ 2, 2 ], [ 3, 2 ] ], [  ], [  ], [ [ 2, 2 ], [ 3, 2 ] ], 
  [ [ 2, 2 ], [ 3, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:83
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,5));
# Expected output:
[ fail, [  ], fail, fail, [ [ 2, 2 ], [ 5, 2 ] ], [ [ 3, 2 ], [ 5, 2 ] ] ]
# But found:
[ [  ], [  ], [  ], [  ], [ [ 2, 2 ], [ 5, 2 ] ], [ [ 3, 2 ], [ 5, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:85
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,7));
# Expected output:
[ fail, [ [ 2, 2 ], [ 7, 2 ] ], fail, fail, [  ], [ [ 2, 2 ], [ 7, 2 ] ] ]
# But found:
[ [  ], [ [ 2, 2 ], [ 7, 2 ] ], [  ], [  ], [  ], [ [ 2, 2 ], [ 7, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:87
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,11));
# Expected output:
[ fail, [ [ 2, 2 ], [ 11, 2 ] ], fail, fail, [ [ 2, 2 ], [ 11, 2 ] ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [  ], [ [ 2, 2 ], [ 11, 2 ] ], [  ], [  ], [ [ 2, 2 ], [ 11, 2 ] ], 
  [ [ 2, 2 ], [ 3, 2 ] ] ]
########
drallenherman commented 4 years ago

@drallenherman added more tests to ensure that diffs are 100% covered. Tried them in wedderga 4.9.5. Some of the diffs are from [ ] becoming fail, but there are some more. So, is all the new output corect?

########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:75
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,-2));
# Expected output:
[ fail, fail, fail, fail, fail, fail ]
# But found:
[ [  ], [ [ infinity, 2 ], [ 2, 2 ] ], [  ], [  ], [  ], [  ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:77
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,-1));
# Expected output:
[ fail, [ [ infinity, 2 ], [ 2, 2 ] ], fail, fail, [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [ [ infinity, 2 ], [ 2, 2 ] ], [ [ infinity, 2 ], [ 2, 2 ] ], [  ], [  ], 
  [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:79
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,2));
# Expected output:
[ fail, [  ], fail, fail, [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [  ], [  ], [  ], [  ], [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:81
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,3));
# Expected output:
[ fail, [ [ 2, 2 ], [ 3, 2 ] ], fail, fail, [ [ 2, 2 ], [ 3, 2 ] ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [  ], [ [ 2, 2 ], [ 3, 2 ] ], [  ], [  ], [ [ 2, 2 ], [ 3, 2 ] ], 
  [ [ 2, 2 ], [ 3, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:83
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,5));
# Expected output:
[ fail, [  ], fail, fail, [ [ 2, 2 ], [ 5, 2 ] ], [ [ 3, 2 ], [ 5, 2 ] ] ]
# But found:
[ [  ], [  ], [  ], [  ], [ [ 2, 2 ], [ 5, 2 ] ], [ [ 3, 2 ], [ 5, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:85
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,7));
# Expected output:
[ fail, [ [ 2, 2 ], [ 7, 2 ] ], fail, fail, [  ], [ [ 2, 2 ], [ 7, 2 ] ] ]
# But found:
[ [  ], [ [ 2, 2 ], [ 7, 2 ] ], [  ], [  ], [  ], [ [ 2, 2 ], [ 7, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:87
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,11));
# Expected output:
[ fail, [ [ 2, 2 ], [ 11, 2 ] ], fail, fail, [ [ 2, 2 ], [ 11, 2 ] ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [  ], [ [ 2, 2 ], [ 11, 2 ] ], [  ], [  ], [ [ 2, 2 ], [ 11, 2 ] ], 
  [ [ 2, 2 ], [ 3, 2 ] ] ]
########

This output is what the program would have returned before #82. It would return a meaningless list of indices, often [ ] but not always, because it would be calculating using the Legendre symbol formulas with numbers that aren't primes or -1's. I assume here this is the output before adding elif lines to check for invalid input, if that's the case this is what I would expect to get.

drallenherman commented 4 years ago

@drallenherman added more tests to ensure that diffs are 100% covered. Tried them in wedderga 4.9.5. Some of the diffs are from [ ] becoming fail, but there are some more. So, is all the new output corect?

########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:75
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,-2));
# Expected output:
[ fail, fail, fail, fail, fail, fail ]
# But found:
[ [  ], [ [ infinity, 2 ], [ 2, 2 ] ], [  ], [  ], [  ], [  ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:77
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,-1));
# Expected output:
[ fail, [ [ infinity, 2 ], [ 2, 2 ] ], fail, fail, [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [ [ infinity, 2 ], [ 2, 2 ] ], [ [ infinity, 2 ], [ 2, 2 ] ], [  ], [  ], 
  [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:79
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,2));
# Expected output:
[ fail, [  ], fail, fail, [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [  ], [  ], [  ], [  ], [  ], [ [ 2, 2 ], [ 3, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:81
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,3));
# Expected output:
[ fail, [ [ 2, 2 ], [ 3, 2 ] ], fail, fail, [ [ 2, 2 ], [ 3, 2 ] ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [  ], [ [ 2, 2 ], [ 3, 2 ] ], [  ], [  ], [ [ 2, 2 ], [ 3, 2 ] ], 
  [ [ 2, 2 ], [ 3, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:83
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,5));
# Expected output:
[ fail, [  ], fail, fail, [ [ 2, 2 ], [ 5, 2 ] ], [ [ 3, 2 ], [ 5, 2 ] ] ]
# But found:
[ [  ], [  ], [  ], [  ], [ [ 2, 2 ], [ 5, 2 ] ], [ [ 3, 2 ], [ 5, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:85
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,7));
# Expected output:
[ fail, [ [ 2, 2 ], [ 7, 2 ] ], fail, fail, [  ], [ [ 2, 2 ], [ 7, 2 ] ] ]
# But found:
[ [  ], [ [ 2, 2 ], [ 7, 2 ] ], [  ], [  ], [  ], [ [ 2, 2 ], [ 7, 2 ] ] ]
########
########> Diff in ~/GITREPS/pkg/wedderga/tst/div-alg.tst:87
# Input is:
List([-2..3],a->LocalIndicesOfRationalSymbolAlgebra(a,11));
# Expected output:
[ fail, [ [ 2, 2 ], [ 11, 2 ] ], fail, fail, [ [ 2, 2 ], [ 11, 2 ] ], [ [ 2, 2 ], [ 3, 2 ] ] ]
# But found:
[ [  ], [ [ 2, 2 ], [ 11, 2 ] ], [  ], [  ], [ [ 2, 2 ], [ 11, 2 ] ], 
  [ [ 2, 2 ], [ 3, 2 ] ] ]
########

This output is what the program would have returned before #82. It would return a meaningless list of indices, often [ ] but not always, because it would be calculating using the Legendre symbol formulas with numbers that aren't primes or -1's. I assume here this is the output before adding elif lines to check for invalid input, if that's the case this is what I would expect to get.

I checked the code and new tests in #83, it is giving correct output.

olexandr-konovalov commented 4 years ago

@drallenherman perfect, thanks (if so, you're also encouraged to actually approve the PR using the "Review" button).