Fix several bugs in ReducingCyclotomicAlgebra.

[angeldelriomateos]

The definition of the fields F1 and F2 have been modified. F1=K^G1 and F2=K^G2 where K=F(E(m)) with F=A[2] and m=A[3]. The error consists in assuming that m coincides with the conductor of K. G1 and G2 are expressed in terms of the information in A[4] which in turn is expresed in terms of m. To define F1 and F2 properly is necessarily to express the information of A[4] in terms of the conductor of K. Another bug fixed in the return for the case where s1=s2=1, i.e. when the algebra is split.

[angeldelriomateos]

I create a new PR #64 where I fixed the bug in ReducyingCyclotomicAlgebra.

[angeldelriomateos]

FactoringCycAlg contains the same bug that I just fixed in ReducingCyclotomicAlgebra. However FactoringCycAlg is now useless because its functionality is integrated in ReducingCyclotomicAlgebra so it should be deleted from div-alg.gi and its declaration in div-alg.gd should be removed. I won't touch it for the moment until the previous PRs about Fixing ReducingCyclotomicAlgebra and ShortWDI have been estabilized.

[codecov[bot]]

Codecov Report

Merging #64 into divalg will increase coverage by 0.04%. The diff coverage is 52.23%.

@@            Coverage Diff             @@
##           divalg      #64      +/-   ##
==========================================
+ Coverage   77.37%   77.42%   +0.04%     
==========================================
  Files          11       11              
  Lines        5313     5319       +6     
==========================================
+ Hits         4111     4118       +7     
+ Misses       1202     1201       -1     

Impacted Files	Coverage Δ
lib/div-alg.gi	74.33% <51.51%> (+0.14%)	:arrow_up:
lib/div-alg.gd	100.00% <100.00%> (ø)

[olexandr-konovalov]

Thanks! Can we add a test for "If not we check whether A1 is cyclotomic and compute the Schur index of A1" case to improve code coverage? You can add another commit to this branch, the test can go e.g. to tst/bugfix.tst.

[angeldelriomateos]

Thanks! Can we add a test for "If not we check whether A1 is cyclotomic and compute the Schur index of A1" case to improve code coverage? You can add another commit to this branch, the test can go e.g. to tst/bugfix.tst.

Sure. I am computing all the Wedderburn decomposition of all the groups of order at most 255. It takes a few hours but not more than 2 hours. I believe that this test will go throught all the parts of ReducingCyclotomicAlgebras, except for the second part where the algebra is splitted into two non-cyclic cyclotomic algebras. For that one need a group of order at least 1920. Maybe SG(1920,1335) uses that part but I am not sure. If not I believe that some group of order 1920 should use it. If not I am sure that I can construct a group of order 3840 that needs to use it, what I don't know is whether GAP can manage such a big group

[angeldelriomateos]

Made some comments about coding style - you can fix that in a separate commit too (my plan is to squash all commits in this PR into one when I will be merging it).

Done. I understand that I don't need to do a new PR. Please check and confirm.

[olexandr-konovalov]

Thanks - yes, you don't need a new PR for that, just keep making new commits until this will be ready, and I will squash them in one commit (but please be sure that you don't make unrelated commits in this PR then).

You did not address yet all my comments yet - two of them still remain under https://github.com/gap-packages/wedderga/pull/64/files (if that's too tedious, I will make them while merging myself).

But more seriously, I just noticed that you are making this PR from your divalg branch - apparently you missed creating a new branch at step 4 from https://github.com/gap-packages/wedderga/wiki. That will make things slightly more complicated if you will decide to work on some other change in parallel, but for now it's fine - I will provide instructions later.

[angeldelriomateos]

Thanks - yes, you don't need a new PR for that, just keep making new commits until this will be ready, and I will squash them in one commit (but please be sure that you don't make unrelated commits in this PR then).

You did not address yet all my comments yet - two of them still remain under https://github.com/gap-packages/wedderga/pull/64/files (if that's too tedious, I will make them while merging myself).

But more seriously, I just noticed that you are making this PR from your divalg branch - apparently you missed creating a new branch at step 4 from https://github.com/gap-packages/wedderga/wiki. That will make things slightly more complicated if you will decide to work on some other change in parallel, but for now it's fine - I will provide instructions later.

Could you be more specific about which comments I did not address? Is it about the testing? If so I don't know how to do it. About the second, when I did the previous pull request about ShortWDI I create a branch called ShortWDI. I guess that I should have done it from ShortDVI-divalg. Should I repeate the commit again from that branch?

[olexandr-konovalov]

Could you be more specific about which comments I did not address? Is it about the testing? If so I don't know how to do it.

About blank lines and missing new line at the very end of the file - they show up as unnecessary diffs in https://github.com/gap-packages/wedderga/pull/64/files - but as I said, if that's too tedious for now, I will fix that myself later.

About the second, when I did the previous pull request about ShortWDI I create a branch called ShortWDI. I guess that I should have done it from ShortDVI-divalg.

No - ShortDVI-divalg is already a branch for an existing PR. If you wanted to do a new PR the correct approach would be to start a new branch off divalg branch:

git checkout divalg
git pull origin divalg
git checkout -b fix-reduceCA

and then commit/push etc. You missed the last of these three comands.

Should I repeate the commit again from that branch?

No, just continue for now here, in the same way as you did in the last commit when you fixed formatting.

[angeldelriomateos]

Could you be more specific about which comments I did not address? Is it about the testing? If so I don't know how to do it.

About blank lines and missing new line at the very end of the file - they show up as unnecessary diffs in https://github.com/gap-packages/wedderga/pull/64/files - but as I said, if that's too tedious for now, I will fix that myself later.

I did it and commit and push it from divalg. Maybe I did something wrong. Anyway, in the future I'll try to not adding unnecessary changes.

[olexandr-konovalov]

Last commit in https://github.com/angeldelriomateos/wedderga/tree/divalg is 9 hours old - is that correct?

[angeldelriomateos]

Last commit in https://github.com/angeldelriomateos/wedderga/tree/divalg is 9 hours old - is that correct?

Yes!

[olexandr-konovalov]

OK! Just to check, git status and git diff do not report any uncommitted changes, right?

[angeldelriomateos]

git status in divalg says that my branch is ahead of origin/divalg by two commitments. git diff says nothing

[olexandr-konovalov]

Ok! I will wait then to hear whether you will manage to find a good example for the test.

[angeldelriomateos]

Ok! I will wait then to hear whether you will manage to find a good example for the test.

I guess that what we should test is that the output of WedderburnInformationInfo now is correct. Already SG(6,1) is a good test. Before the changes in PR #63 the output was [[1,Rationals],[1,Rationals], [2,Rationals,3,[2,2,0]]]. After this change the output should be [[1,Rationals],[1,Rationals], [2,Rationals]].

After I did that commitment I noticed a bug in ReducingCyclotomicAlgebra causing a non correct behaviour in SG(24,1) and some groups of order 48 and of order 480. The aim of PR #64 was to fix that. The output of WedderburnDecompositionInfo(QG) with the changes in PR #63 and PR #64 for QG=GroupRing(Rationals,G) with G=SG(24,1) should be [ [ 1, Rationals ], [ 1, Rationals ], [ 1, GaussianRationals ], [ 2, Rationals ], [ 1, CF(8) ], [ 1, Rationals, 6, [ 2, 5, 3 ] ], [ 1, GaussianRationals, 12, [ 2, 5, 9 ] ] ]

This is another testing group. Also testing that WedderburnDecomposition(QG) for G running on all the groups of order 480 does not crash should be a good test. I already did this test locally.

[angeldelriomateos]

I also verified that WedderburnDecompositionInfo does not crash for all the groups of order at most 100 and compare the difference in runtime with the old version 225168 and the new version: 256255 miliseconds. So the new version is about 15% slower but the output is much more precise. So I believe it is worth this slowing down in speed to get a better result.

There is a third version that I want to try with an ever better output but I guess that it should be much slower. I am going to do that test now.

Do you want me to share my testing file?

[drallenherman]

Thanks! Can we add a test for "If not we check whether A1 is cyclotomic and compute the Schur index of A1" case to improve code coverage? You can add another commit to this branch, the test can go e.g. to tst/bugfix.tst.

Sure. I am computing all the Wedderburn decomposition of all the groups of order at most 255. It takes a few hours but not more than 2 hours. I believe that this test will go throught all the parts of ReducingCyclotomicAlgebras, except for the second part where the algebra is splitted into two non-cyclic cyclotomic algebras. For that one need a group of order at least 1920. Maybe SG(1920,1335) uses that part but I am not sure. If not I believe that some group of order 1920 should use it. If not I am sure that I can construct a group of order 3840 that needs to use it, what I don't know is whether GAP can manage such a big group

I loaded #64 successfully. I am searching groups of order 1920 for a good example. Using an older branch since WDI does not use ReducingCyclotomicAlgebras. SG(1920,236125) produces a 4-generated Galois group, and RCA returns fail. It should test the second part of RCA.
Btw, this group uncovered a bug in DefiningCharacterOfCyclotomicAlgebra - it also needs adjustment for an arbitrary number of generators in one part. The current version was designed for only up to 3 generators.

[angeldelriomateos]

I am searching groups of order 1920 for a good example. Using an older branch since WDI does not use ReducingCyclotomicAlgebras. SG(1920,236125) produces a 4-generated Galois group, and RCA returns fail. It should test the second part of RCA.

Btw, this group uncovered a bug in DefiningCharacterOfCyclotomicAlgebra - it also needs adjustment for an arbitrary number of generators in one part. The current version was designed for only up to 3 generators.

I calculated the WedderburnDecompositionInfo of QG for that group with the new version. However, I think that for this group ReducingCyclotomicAlgebra does nothing, at least for the component that one can expect to do something. The component on which this could hold is as follow: [ 1, NF(5,[ 1, 4 ]), 120, [ [ 2, 11, 60 ], [ 2, 19, 60 ], [ 2, 29, 60 ], [ 2, 31, 60 ] ], [ [ 60, 60, 0 ], [ 60, 0 ], [ 0 ] ] ] ]. The only way to split this is separating the fourth Galois generator of the other three. For this to really do something one requires the the fix field F2 of the fourth generator be a cyclotomic extension of the center, and it is not.

[olexandr-konovalov]

@drallenherman wrote

Do you want me to share my testing file?

yes, sure! Perhaps you can create a new test file tst/divalg.tst and start to assemble a thorough and consistent collection of tests there. At the moment, we are testing things from divalg mainly due to manual examples, and there we are constrained by making some illustrative examples. If you will have a pure test file, the purpose of it is to do more tests, and you can easily put there e.g. WedderburnDecomposition(QG) for G running on all the groups of order 480. You can suppress variable outputs with ;;. And also note that free Travis CI services have time limits - a test is terminated if it runs about an hour or long, and it is terminated if it runs 10 minutes without any output. So we will be able to catch some performance regressions too.

When you add a new file, you need to call

git add tst/divalg.tst

(adjust path/filename as needed) before git commit, to put that file under version control.

[drallenherman]

I am searching groups of order 1920 for a good example. Using an older branch since WDI does not use ReducingCyclotomicAlgebras. SG(1920,236125) produces a 4-generated Galois group, and RCA returns fail. It should test the second part of RCA.

Btw, this group uncovered a bug in DefiningCharacterOfCyclotomicAlgebra - it also needs adjustment for an arbitrary number of generators in one part. The current version was designed for only up to 3 generators.

I calculated the WedderburnDecompositionInfo of QG for that group with the new version. However, I think that for this group ReducingCyclotomicAlgebra does nothing, at least for the component that one can expect to do something. The component on which this could hold is as follow: [ 1, NF(5,[ 1, 4 ]), 120, [ [ 2, 11, 60 ], [ 2, 19, 60 ], [ 2, 29, 60 ], [ 2, 31, 60 ] ], [ [ 60, 60, 0 ], [ 60, 0 ], [ 0 ] ] ] ]. The only way to split this is separating the fourth Galois generator of the other three. For this to really do something one requires the the fix field F2 of the fourth generator be a cyclotomic extension of the center, and it is not.

Yes this is why RCA returns fail. But that does not mean it would not improve coverage result of travis test. I am looking for a better example of order 1920.

[drallenherman]

I think there is still a bug causing incorrect output in ReducingCyclotomicAlgebra in branch #64: Here is what it gives:

gap> G:=SmallGroup(1920,235974); <pc group of size 1920 with 9 generators> gap> W:=WedderburnDecompositionInfo(GroupRing(Rationals,G));; W[Length(W)]; [ 2, NF(120,[ 1, 11, 31, 101 ]), 120, [ [ 2, 11, 0 ], [ 2, 31, 0 ] ], [ [ 0 ] ] ] gap> ReducingCyclotomicAlgebra(W[Length(W)]); [ 4, NF(120,[ 1, 11, 31, 101 ]), 120, [ 2, 11, 0 ] ]

The problem is the root of unity in the third entry of the output. <31> fixes E(120)^30 = E(4) so it should be a 4. And the 11 should be reduced mod 4 but that will not cause errors later. As it stands now this will not pass the "genuine cyclotomic algebra" test.

Still looking for a better example of order 1920. Tomorrow I will start a new PR to take care of: 1) Modify DefiningCharacterOfCyclotomicAlgebra to handle Galois groups with arbitrary number of generators; 2) Modify SchurIndexByCharacter to use SimpleComponentByCharacterDescent; 3) Add/revise documentation.

[angeldelriomateos]

gap> W:=WedderburnDecompositionInfo(GroupRing(Rationals,G));; W[Length(W)]; [ 2, NF(120,[ 1, 11, 31, 101 ]), 120, [ [ 2, 11, 0 ], [ 2, 31, 0 ] ], [ [ 0 ] ] ] gap> ReducingCyclotomicAlgebra(W[Length(W)]); [ 4, NF(120,[ 1, 11, 31, 101 ]), 120, [ 2, 11, 0 ] ]

The problem is the root of unity in the third entry of the output. <31> fixes E(120)^30 = E(4) so it should be a 4. And the 11 should be reduced mod 4 but that will not cause errors later. As it stands now this will not pass the "genuine cyclotomic algebra" test.

31 does not fix E(4). Actually the fixed field by such automorphism is NF(120,[1,31]) whose conductor is 120. So the reduction is correct.

[drallenherman]

gap> W:=WedderburnDecompositionInfo(GroupRing(Rationals,G));; W[Length(W)]; [ 2, NF(120,[ 1, 11, 31, 101 ]), 120, [ [ 2, 11, 0 ], [ 2, 31, 0 ] ], [ [ 0 ] ] ] gap> ReducingCyclotomicAlgebra(W[Length(W)]); [ 4, NF(120,[ 1, 11, 31, 101 ]), 120, [ 2, 11, 0 ] ] The problem is the root of unity in the third entry of the output. <31> fixes E(120)^30 = E(4) so it should be a 4. And the 11 should be reduced mod 4 but that will not cause errors later. As it stands now this will not pass the "genuine cyclotomic algebra" test.

31 does not fix E(4). Actually the fixed field by such automorphism is NF(120,[1,31]) whose conductor is 120. So the reduction is correct.

You should not use the conductor to define this cyclotomic algebra - F(E(120)) clearly has degree 4 over F and <31> has order 2, so the order of the root of unity used for the cyclotomic extension should be reduced to the order of the first power of E(120) fixed by the <11>. So not to 4 but to 10.

[drallenherman]

Let me try again. Reduce to the first power of E(120) fixed by <31>. This is E(30). So the 120 should change to 30. <11> acts nontrivially on F(E(30)) and this has degree 2 over F, so the result is a genuine cyclotomic algebra.

[angeldelriomateos]

Let me try again. Reduce to the first power of E(120) fixed by <31>. This is E(30). So the 120 should change to 30. <11> acts nontrivially on F(E(30)) and this has degree 2 over F, so the result is a genuine cyclotomic algebra.

The fixed field of CF(120) by <31> contains CF(30) properly because 31 has order 2 modulo 120 and CF(120)/CF(30) has degree 4.

But F=A[2] so F(E(30)) is a proper extension of CF(30). The output of WDI tells us F(E(120))/F has degree 4 with Galois group <11>x <31>. RCA checks the field fixed by <31> is a cyclotomic extension of F, and this is F(E(30)). So the 120 needs to be a 30 for the result of RCA to be a well defined cyc alg.

[drallenherman]

But F=A[2] so F(E(30)) is a proper extension of CF(30). The output of WDI tells us F(E(120))/F has degree 4 with Galois group <11>x <31>. RCA checks the field fixed by <31> is a cyclotomic extension of F, and this is F(E(30)). So the 120 needs to be a 30 for the result of RCA to be a well defined cyc alg.

[angeldelriomateos]

But F=A[2] so F(E(30)) is a proper extension of CF(30). The output of WDI tells us F(E(120))/F has degree 4 with Galois group <11>x <31>. RCA checks the field fixed by <31> is a cyclotomic extension of F, and this is F(E(30)). So the 120 needs to be a 30 for the result of RCA to be a well defined cyc alg.

I understand now. You are right. Let me revise the code, so that I can fix this.

[drallenherman]

@drallenherman wrote

Do you want me to share my testing file?

yes, sure! Perhaps you can create a new test file tst/divalg.tst and start to assemble a thorough and consistent collection of tests there. At the moment, we are testing things from divalg mainly due to manual examples, and there we are constrained by making some illustrative examples. If you will have a pure test file, the purpose of it is to do more tests, and you can easily put there e.g. WedderburnDecomposition(QG) for G running on all the groups of order 480. You can suppress variable outputs with ;;. And also note that free Travis CI services have time limits - a test is terminated if it runs about an hour or long, and it is terminated if it runs 10 minutes without any output. So we will be able to catch some performance regressions too.

When you add a new file, you need to call

git add tst/divalg.tst

(adjust path/filename as needed) before git commit, to put that file under version control.

Alexander, I've been working on the adjustments for the new issue #65 I raised this morning. I had trouble creating a new pull request on Github Desktop so I've put the initial changes for this issue in my local version of PR#59. It says I have to "publish" my changes now before it will let me create a new pull request. So just ignore my new commit to PR#59 (if you can) hopefully it is just a necessary step for me to get PR#65 created.

[drallenherman]

@drallenherman wrote

Do you want me to share my testing file?

yes, sure! Perhaps you can create a new test file tst/divalg.tst and start to assemble a thorough and consistent collection of tests there. At the moment, we are testing things from divalg mainly due to manual examples, and there we are constrained by making some illustrative examples. If you will have a pure test file, the purpose of it is to do more tests, and you can easily put there e.g. WedderburnDecomposition(QG) for G running on all the groups of order 480. You can suppress variable outputs with ;;. And also note that free Travis CI services have time limits - a test is terminated if it runs about an hour or long, and it is terminated if it runs 10 minutes without any output. So we will be able to catch some performance regressions too. When you add a new file, you need to call

git add tst/divalg.tst

(adjust path/filename as needed) before git commit, to put that file under version control.

Alexander, I've been working on the adjustments for the new issue #65 I raised this morning. I had trouble creating a new pull request on Github Desktop so I've put the initial changes for this issue in my local version of PR#59. It says I have to "publish" my changes now before it will let me create a new pull request. So just ignore my new commit to PR#59 (if you can) hopefully it is just a necessary step for me to get PR#65 created.

It seems I successfully created a branch #66 for Issue #65 called PR/59 based on the old #59. At least the new branch has all my changes for Issue #65. So once Angel is finished with #64 we should be able to merge them, the changes are in different places so not likely to be in conflict.

[angeldelriomateos]

I think I finished fixing the issue rised by Allen with the use of conductor. I am doing some test before doing my commit. I am not keeping track from/to which branch I should push this commit. In my opinion it should be a commitment from divalg because it is a modification in on the functions of div.alg.gi

[olexandr-konovalov]

@drallenherman my understanding is that all commits in this PR are made into your checkout of n divalg and the fix mentioned in the last comment is made on top of them - please continue in divalg then. After we will merge this PR (hopefully tonight?) I will help you to reset it.

[angeldelriomateos]

Besides modifications in ReducingCyclotomicAlgebra I modified IsCyclotomicExtension so that the output for input (K,F) now is not true/false but a non-negative integer n so that if n=0 then the K/F is not a cyclotomic extension of abelian number fields and if n is is positive then K=F(E(n)). So somehow the name is missleading. However I have not changed yet for convenience.

[olexandr-konovalov]

@angeldelriomateos it often happens that a PR evolved beyond its initial title - a good practice is to update the initial comment above to ensure that it contains an adequate description, especially if the thread becomes very long.

[angeldelriomateos]

@angeldelriomateos it often happens that a PR evolved beyond its initial title - a good practice is to update the initial comment above to ensure that it contains an adequate description.

Should I understand that I have to use PR #64 and modified the title to include the new fixing?

[angeldelriomateos]

At this moment I am doing the commit in the branch divalg. Is that correct?

[angeldelriomateos]

I pushed the new commitment. So now PR #64 has three commits of myself. I have run WedderburnDecompositionInfo(QG) for all the groups of order at most 100 and all the groups of order 480. At least it didn't crash. I wait for your comments.

[olexandr-konovalov]

Should I understand that I have to use PR #64 and modified the title to include the new fixing?

yes - if you click "Edit" button in the top part of this page, to the right of the commit title.

At this moment I am doing the commit in the branch divalg. Is that correct?

yes, the new commit is where I have suggested to put it, thanks!

The title of the last commit "Fix bug in ReducingCyclotomicAlgebra..." starts with the same string as the 1st one. Are they fixing different bugs, or still can be glued into one commit?

Tests just pass - good news!

[drallenherman]

I am searching groups of order 1920 for a good example. Using an older branch since WDI does not use ReducingCyclotomicAlgebras. SG(1920,236125) produces a 4-generated Galois group, and RCA returns fail. It should test the second part of RCA.

Btw, this group uncovered a bug in DefiningCharacterOfCyclotomicAlgebra - it also needs adjustment for an arbitrary number of generators in one part. The current version was designed for only up to 3 generators.

I calculated the WedderburnDecompositionInfo of QG for that group with the new version. However, I think that for this group ReducingCyclotomicAlgebra does nothing, at least for the component that one can expect to do something. The component on which this could hold is as follow: [ 1, NF(5,[ 1, 4 ]), 120, [ [ 2, 11, 60 ], [ 2, 19, 60 ], [ 2, 29, 60 ], [ 2, 31, 60 ] ], [ [ 60, 60, 0 ], [ 60, 0 ], [ 0 ] ] ] ]. The only way to split this is separating the fourth Galois generator of the other three. For this to really do something one requires the the fix field F2 of the fourth generator be a cyclotomic extension of the center, and it is not.

Yes this is why RCA returns fail. But that does not mean it would not improve coverage result of travis test. I am looking for a better example of order 1920.

There seems no good example for RCA in a group of order 1920.
But we can test it directly on the following cyclotomic algebra: A:=[1,Rationals,120,[[2,31,0],[2,61,0],[4,73,0],[2,41,0]],[[60,0,0],[0,0],[60]]]; This separates as a tensor product of two length 5 cyclotomic algebras. By playing with the positions of 60s and 0s in the last part we can test iterated applications of RCA too.

[angeldelriomateos]

One more thing. We are not making any real use of FactoringCycAlg. This is why I have not been paying attention to it. Actually its code is very similar to ReducingCyclotomicAlgebra and its use for use is actually included in ReducingCyclotomicAlgebra. I think that we should drop this function. In case it stay it should be be fixed because it contains the same bugs that ReducingCA had before the last fixing.

[olexandr-konovalov]

I think that we should drop this function.

yes, "deleted code is debugged code" - but in a separate PR perhaps? Because you're working in divalg, I don't want to keep this PR unmerged for much longer.

[angeldelriomateos]

I think that we should drop this function.

yes, "deleted code is debugged code" - but in a separate PR perhaps? Because you're working in divalg, I don't want to keep this PR unmerged for much longer.

Ok, so now I will modify the name of IsCyclotomicExtension to CyclotomicExtensionGenerator. In a separate PR I will deal with eliminating FactoringCycAlg

[olexandr-konovalov]

@angeldelriomateos please also clarify what I've asked earlier:

The title of the last commit "Fix bug in ReducingCyclotomicAlgebra..." starts with the same string as the 1st one. Are they fixing different bugs, or still can be glued into one commit?

[angeldelriomateos]

There seems no good example for RCA in a group of order 1920. But we can test it directly on the following cyclotomic algebra: A:=[1,Rationals,120,[[2,31,0],[2,61,0],[4,73,0],[2,41,0]],[[60,0,0],[0,0],[60]]]; This separates as a tensor product of two length 5 cyclotomic algebras. By playing with the positions of 60s and 0s in the last part we can test iterated applications of RCA too.

Playing with similar algebras I discovered a possible bug. Namely I was considering the following algebra: A := [1,Rationals,120,[[2,31,0],[2,61,0],[2,41,0],[4,97,0]],[[60,0,0],[0,0],[60]]]; ReducingCyclotomicAlgebra crashes when applied to this algebra. I investigated why and it is related of when trying to compute the Schur index of one of the factors discovered. Before resuming ReducingCyclotomicAlgebra manage to factor A as tensor product of A1=[ 1, Rationals, 8, [ [ 2, 7, 0 ], [ 2, 5, 0 ] ], [ [ 4 ] ] ] and A2=[ 1, Rationals, 30, [ [ 2, 11, 0 ], [ 4, 7, 0 ] ], [ [ 15 ] ] ] The ReducingCyclotomicAlgebra computes the Schur index of A1 and A2. The Schur index of A1 is 1 but it crashes when calculating the Schur index of A2. Namely it crashes when trying to compute LocalIndexAtTwoByCharacter. Allen, can you check where is the problem?

[olexandr-konovalov]

Do you mean a problem with the code from this PR? In divalg branch from https://github.com/gap-packages/wedderga/tree/divalg I have

gap> A := [1,Rationals,120,[[2,31,0],[2,61,0],[2,41,0],[4,97,0]],[[60,0,0],[0,0],[60]]];
[ 1, Rationals, 120, [ [ 2, 31, 0 ], [ 2, 61, 0 ], [ 2, 41, 0 ], [ 4, 97, 0 ] ], 
  [ [ 60, 0, 0 ], [ 0, 0 ], [ 60 ] ] ]
gap> ReducingCyclotomicAlgebra(A);
[ 4, Rationals, 30, [ [ 2, 11, 0 ], [ 4, 7, 0 ] ], [ [ 15 ] ] ]
gap> A1:=[ 1, Rationals, 8, [ [ 2, 7, 0 ], [ 2, 5, 0 ] ], [ [ 4 ] ] ];
[ 1, Rationals, 8, [ [ 2, 7, 0 ], [ 2, 5, 0 ] ], [ [ 4 ] ] ]
gap> ReducingCyclotomicAlgebra(A1);
fail
gap> A2:=[ 1, Rationals, 30, [ [ 2, 11, 0 ], [ 4, 7, 0 ] ], [ [ 15 ] ] ];
[ 1, Rationals, 30, [ [ 2, 11, 0 ], [ 4, 7, 0 ] ], [ [ 15 ] ] ]
gap> ReducingCyclotomicAlgebra(A2);
fail

[olexandr-konovalov]

@angeldelriomateos also what is "crashes" - GAP terminates completely (what in GAP we usually mean by crash e.g. in release announcements about fixed bugs that may lead to crashes), or enters a break loop (brk>prompt)?

[angeldelriomateos]

Do you mean a problem with the code from this PR? In divalg branch from https://github.com/gap-packages/wedderga/tree/divalg I have

gap> A := [1,Rationals,120,[[2,31,0],[2,61,0],[2,41,0],[4,97,0]],[[60,0,0],[0,0],[60]]];
[ 1, Rationals, 120, [ [ 2, 31, 0 ], [ 2, 61, 0 ], [ 2, 41, 0 ], [ 4, 97, 0 ] ], 
  [ [ 60, 0, 0 ], [ 0, 0 ], [ 60 ] ] ]
gap> ReducingCyclotomicAlgebra(A);
[ 4, Rationals, 30, [ [ 2, 11, 0 ], [ 4, 7, 0 ] ], [ [ 15 ] ] ]
gap> A1:=[ 1, Rationals, 8, [ [ 2, 7, 0 ], [ 2, 5, 0 ] ], [ [ 4 ] ] ];
[ 1, Rationals, 8, [ [ 2, 7, 0 ], [ 2, 5, 0 ] ], [ [ 4 ] ] ]
gap> ReducingCyclotomicAlgebra(A1);
fail
gap> A2:=[ 1, Rationals, 30, [ [ 2, 11, 0 ], [ 4, 7, 0 ] ], [ [ 15 ] ] ];
[ 1, Rationals, 30, [ [ 2, 11, 0 ], [ 4, 7, 0 ] ], [ [ 15 ] ] ]
gap> ReducingCyclotomicAlgebra(A2);
fail

Yes and no. In this PR I have been modifying mostly ReducingCyclotomicAlgebra. One of the modifications in the first commit added the use of SchurIndex at the very end. There is where the crash happens. So the problem should be in the use of SchurIndex, but my investigation says that the problem appears with using LocalIndicesAtTwoByCharacter, which is called by SchurIndex.

I think that in your calculations you are using a previous version of ReducingCyclotomicAlgebra which does not use SchurIndex. This is why it does not crash for you. The point is that if one cannot apply SchurIndex to the factors this is useless.

[olexandr-konovalov]

Yes, my calculations use divalg branch from https://github.com/gap-packages/wedderga/tree/divalg - so that's indeed the clean state of this branch in the main wedderga repository.

But I have now checked out your branch - into a separate angeldelriomateos-divalg local branch, because I have already my own divalg and want to keep them separately:

git checkout -b angeldelriomateos-divalg divalg
git pull https://github.com/angeldelriomateos/wedderga.git divalg

Now indeed I am in your branch

$ git branch
* angeldelriomateos-divalg
  divalg
  master

with git log showing this as last commit:

commit c841706a00ba0671489b3950f53750a2dfe82ca2 (HEAD -> angeldelriomateos-divalg)
Author: Ángel del Río <adelrio@um.es>
Date:   Tue Apr 14 01:14:37 2020 +0200

    Changing the name of IsCyclotomicExtension by CyclotomicExtensionGenerator
    The old IsCyclotomicExtension was a boolean, the new one outputs a positive integer n if the input is formed by K=F(E(n)) and an
    abelian number field F.
    An unrelated fixing replaces E(con) by E(m), that was unnoticed in the previous commit

and the same example working with the same output.

Are you sure you don't have any uncommitted changes? What git diff reports?

[olexandr-konovalov]

Anyhow, I am wrapping up for tonight. If you think that the example is correct and should work in any case, please add it to tst/div-alg.tst and commit too. Then we can check it with Travis and be sure that we all see the same version of the code.

[drallenherman]

There seems no good example for RCA in a group of order 1920. But we can test it directly on the following cyclotomic algebra: A:=[1,Rationals,120,[[2,31,0],[2,61,0],[4,73,0],[2,41,0]],[[60,0,0],[0,0],[60]]]; This separates as a tensor product of two length 5 cyclotomic algebras. By playing with the positions of 60s and 0s in the last part we can test iterated applications of RCA too.

Playing with similar algebras I discovered a possible bug. Namely I was considering the following algebra: A := [1,Rationals,120,[[2,31,0],[2,61,0],[2,41,0],[4,97,0]],[[60,0,0],[0,0],[60]]]; ReducingCyclotomicAlgebra crashes when applied to this algebra. I investigated why and it is related of when trying to compute the Schur index of one of the factors discovered. Before resuming ReducingCyclotomicAlgebra manage to factor A as tensor product of A1=[ 1, Rationals, 8, [ [ 2, 7, 0 ], [ 2, 5, 0 ] ], [ [ 4 ] ] ] and A2=[ 1, Rationals, 30, [ [ 2, 11, 0 ], [ 4, 7, 0 ] ], [ [ 15 ] ] ] The ReducingCyclotomicAlgebra computes the Schur index of A1 and A2. The Schur index of A1 is 1 but it crashes when calculating the Schur index of A2. Namely it crashes when trying to compute LocalIndexAtTwoByCharacter. Allen, can you check where is the problem?

Using pr/65 or divalg branches, I do not see a problem with A2. The local indices of A2 are 2 at p=3 and 5. SchurIndex returns 2 for A2 and 1 for A1. So the issue must be somewhere in your current branch.