Update DecomposeCyclotomicAlgebra and improve KillingCocycle

[drallenherman]

-revised DecomposeCyclotomicAlgebra to use KillingCocycle -small improvement to KillingCocycle

[drallenherman]

-revised DecomposeCyclotomicAlgebra to use KillingCocycle -small improvement to KillingCocycle

Tests are failing just because of one example I had to update in divalg.xml. I had trouble running makedoc to work.

[angeldelriomateos]

I tried to revised the differences but github shows to many. It seems that the fact that you use windows produces makes the system to recognize as differences some control characters. In any case I revised the code of KillingCocycle and DecomposeCyclotomicAlgebra. It seems that in the first the only change is excluding j=i at some point. If I understood correctly DCA it only produces some output if either A[5] is formed of zeros so that the output is a list of cyclic (non-necesarily cyclotomic algebras) or A[4] has length 2 and some additional condition hold which I still do not revise carefully. I fill that I can trust you on that.

[drallenherman]

I tried to revised the differences but github shows to many. It seems that the fact that you use windows produces makes the system to recognize as differences some control characters. In any case I revised the code of KillingCocycle and DecomposeCyclotomicAlgebra. It seems that in the first the only change is excluding j=i at some point. If I understood correctly DCA it only produces some output if either A[5] is formed of zeros so that the output is a list of cyclic (non-necesarily cyclotomic algebras) or A[4] has length 2 and some additional condition hold which I still do not revise carefully. I fill that I can trust you on that.

DCA tries to use KC to obtain a tensor factorization into cyclic algebras first, this can work when Length(A[4]) is arbitrary. If still A[5] is not zero, and Length(A[4])=2 it tries to get the tensor factorization using tricks from the earlier version. It looks to multiply one generator by some suitable nonzero b so that (bv)u=u(bv) or v(bu)=(bu)v. It was failing for four groups of order 240 but I added a search of nonzero sums of two nth roots of unity and this to be enough to get a tensor factorization for any component of a groups of order up to 300. I will push that now, but the tests will still fail until I can divalg.xml to run through makedoc.g.

[codecov[bot]]

Codecov Report

Merging #80 into divalg will increase coverage by 0.88%. The diff coverage is 82.60%.

@@            Coverage Diff             @@
##           divalg      #80      +/-   ##
==========================================
+ Coverage   82.50%   83.38%   +0.88%     
==========================================
  Files          11       11              
  Lines        5189     5177      -12     
==========================================
+ Hits         4281     4317      +36     
+ Misses        908      860      -48     

Impacted Files	Coverage Δ
lib/div-alg.gi	92.99% <82.60%> (+2.82%)	:arrow_up:

[olexandr-konovalov]

1. GitHub reports that some checks were not successful. However, we see that Travis tests passed, but Codecov shows that only 72% of diffs were tested. We may decide to merge it as such, or try to add more tests.

2. Low coverage may be actually a result of a number of whitespace changes due to your Atom setting, which I suggested to change earlier - apparently it's still unchanged. Due to whitespace changes, lines with only whitespace changes are still considered as modified, and if they are not covered by tests, that distorts the statistics.

3. Please fix your Atom editor to avoid this happening again: see https://ourcodeworld.com/articles/read/514/how-to-remove-trailing-whitespace-on-save-in-atom-editor which shows how to navigate to the menu responsible for this behaviour. What you have to do there is to UNCHECK "remove trailing whitespace" box to ensure that your edits only modify lines which you have actually modified.

4. @angeldelriomateos You can see actual changes, if you hides whitespace changes as shown here:

5. The second commit in this PR includes main.gi with only whitespace changes. During rebasing of this branch on top of most recent commits in divalg, one should restore main.gi to remain unchanged.

6. I wonder how did it happen that main.gi was committed. First, I recommend to call git diff before commit to see what are you going to commit. Second, perhaps you called git commit -a to commit all modified files in one go. This is a useful shortcut if you really sure that all modified files belong to that commit, but if you want more granularity, you can do e.g.

   git add lib/div-alg.gi
   git add tst/div-alg.tst
   git commit

   and then only these two files will be committed, but no other modified files.

7. I also suggest that once we merge divalg, I will systematically remove trailing whitespaces from all files. I can not do it now, since it will make harder rebasing changes which are in progress.

[olexandr-konovalov]

P.S. @drallenherman if you give me write access to your fork, I am happy to rebase and refactor this PR myself.

[olexandr-konovalov]

@drallenherman ah, ignore the write access request - I've noticed that you are working on a branch in the main repository. So I can push to that branch anyway. Just let me know which way you prefer to proceed.

[drallenherman]

@drallenherman ah, ignore the write access request - I've noticed that you are working on a branch in the main repository. So I can push to that branch anyway. Just let me know which way you prefer to proceed.

Sorry. I actually got to drive to the university and work from my office today. And that took some time! I can rebase to the updated divalg and adjust the test here. I can also remove the two cancelling commits to main.gi - those were just for testing purposes. I'm unsure what will happen with the whitespace issue when I push it back though. .

[olexandr-konovalov]

Seems the rebase did not go the way I expected - ideally, there should be no that merge commit. Do you mind me making some changes, and then you will have to reset your local branch for this PR to sync with remote?

[drallenherman]

Seems the rebase did not go the way I expected - ideally, there should be no that merge commit. Do you mind me making some changes, and then you will have to reset your local branch for this PR to sync with remote?

Sorry. Was in a thesis defense for a couple of hours. Go ahead. There were changes passed upstream that came from #81 to divalg so that's the reason for the merge commit.

[olexandr-konovalov]

@drallenherman ok, I will try tonight or tomorrow morning. That's what I mean by rebase (which would avoid that merge change):

git checkout divalg
git pull
git checkout update-DecomposeCA
git rebase divalg

The last command could be also used with -i for an interactive rebase, when you edit a file which describes rebase steps, and then git will process it after closing the editor.

[olexandr-konovalov]

I have rebased this and made some changes in the revisions history. I will continue tomorrow. I will also have to update examples in the xml documentation from wedderga07.tst file - the latter is autogenerated, and any changes there will be overwritten when the manual is built (see #70). But now there are almost no whitespace changes, and @angeldelriomateos can see what has been changed easier.

@drallenherman If you want to work with the new version, do this:

git fetch origin
git checkout update-DecomposeCA
git reset --hard origin/update-DecomposeCA 

This will overwrite all your local history in update-DecomposeCA from the remote one.

Remarkably, now the test check only 63.02% of diffs, what is even less than before. I think that after removing whitespace changes we have an actual picture, while before that we had false positives. So, if you can add any new examples to improve coverage, you're welcome! See lines marked with red at https://codecov.io/gh/gap-packages/wedderga/compare/055c2b6c37c24091768927be8c9883e487046279...7024cce221d183ae2cbd765689d13d090a07456e/diff

[drallenherman]

I have rebased this and made some changes in the revisions history. I will continue tomorrow. I will also have to update examples in the xml documentation from wedderga07.tst file - the latter is autogenerated, and any changes there will be overwritten when the manual is built (see #70). But now there are almost no whitespace changes, and @angeldelriomateos can see what has been changed easier.

@drallenherman If you want to work with the new version, do this:

git fetch origin
git checkout update-DecomposeCA
git reset --hard origin/update-DecomposeCA 

This will overwrite all your local history in update-DecomposeCA from the remote one.

Remarkably, now the test check only 63.02% of diffs, what is even less than before. I think that after removing whitespace changes we have an actual picture, while before that we had false positives. So, if you can add any new examples to improve coverage, you're welcome! See lines marked with red at https://codecov.io/gh/gap-packages/wedderga/compare/055c2b6c37c24091768927be8c9883e487046279...7024cce221d183ae2cbd765689d13d090a07456e/diff

All right. I did that. Most of the red lines of code are actually from easy cases. We need one Length 4 example:

gap> DecomposeCyclotomicAlgebra([1,NF(7,[1,2,4]),18,[3,2,9]]); [ NF(7,[ 1, 2, 4 ]),NF(63,[ 1, 37, 46 ]),[ -1 ] ]

and one Length 5 example with all 0's in A[5].

gap> A:=[1,NF(7,[1,6]),84,[[2,13,42],[2,29,63],[2,43,0]],[[0,0],[0]]]; [ 1, NF(7,[ 1, 6 ]), 84, [ [ 2, 13, 42 ], [ 2, 29, 63 ], [ 2, 43, 0 ] ], [ [ 0, 0 ], [ 0 ] ] ] gap> DecomposeCyclotomicAlgebra(A); [ [ NF(7,[ 1, 6 ]), CF(7), [ -1 ] ], [ NF(7,[ 1, 6 ]), NF(21,[ 1, 13 ]), [ -E(4) ] ], [ NF(7,[ 1, 6 ]), NF(28,[ 1, 13 ]), [ 1 ] ] ]

The other lines will be much harder to cover. I blotted out the last 4 cases this morning and no group of order 160, 300, or 720 requires them.

[olexandr-konovalov]

Thanks @drallenherman - with new examples, the diff coverage is now 83.19%, that's great!

[olexandr-konovalov]

@drallenherman seems ready to be merged. Perhaps @angeldelriomateos would like to be updated on this first?

[angeldelriomateos]

Yes ple

@drallenherman seems ready to be merged. Perhaps @angeldelriomateos would like to be updated on this first?

Yes, please

[drallenherman]

Thanks @drallenherman - with new examples, the diff coverage is now 83.19%, that's great!

Yes it's better. I noticed the 18,[3,2,9] in the Length 4 example should be 6,[3,2,3] for the extension to have the right degree in the result. I will make a commit to correct that.

[olexandr-konovalov]

@drallenherman your new test works. I will absorb it with the previous commit while merging this.

[drallenherman]

@drallenherman your new test works. I will absorb it with the previous commit while merging this.

All right. I don't have anything more for this branch.

[olexandr-konovalov]

@drallenherman OK, I've force-pushed edited history. You will have to reset your local branch again should you ever need to use it (but perhaps not - we can just merge it).