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

Fix GAPDoc syntax in the documentation #59

Closed olexandr-konovalov closed 4 years ago

olexandr-konovalov commented 4 years ago

It happened that nobody checked that gap makedoc.g actually builds the manual. I have fixed XML syntax and it now builds, and examples are extracted into test files. These test files must also be regenerated each time the manual is updated, and committed too, in order to be tested.

olexandr-konovalov commented 4 years ago

@drallenherman @angeldelriomateos now that's very interesting. We are able to test manual examples, and here what's happening: they all pass in the GAP development version, however they fail in GAP release branches:

Click on the links and scroll to the bottom to see diffs. For example, what we have in stable-4.11 with fractional dimensions(!):

########> Diff in /home/travis/build/gap-packages/wedderga/gaproot/pkg/wedderg\
a/tst/wedderga07.tst:162
# Input is:
SimpleComponentByCharacterDescent(Rationals,G,8);
# Expected output:
[ 21/2, GaussianRationals, 12, [ 2, 5, 9 ] ]
# But found:
[ 21/2, GaussianRationals, 12, [ 2, 5, 3 ] ]
########
########> Diff in /home/travis/build/gap-packages/wedderga/gaproot/pkg/wedderg\
a/tst/wedderga07.tst:186
# Input is:
WedderburnDecompositionByCharacterDescent(Rationals,G);
# Expected output:
[ [ 1, Rationals ], [ 1, Rationals ], [ 12, Rationals ], [ 12, Rationals ],
[ 13, Rationals ], [ 13, Rationals ], [ 16, NF(13,[ 1, 3, 9 ]) ],
[ 16, NF(13,[ 1, 3, 9 ]) ], [ 26, Rationals ], [ 26, Rationals ],
[ 26, NF(8,[ 1, 3 ]) ], [ 26, NF(8,[ 1, 3 ]) ], [ 27, Rationals ],
[ 27, Rationals ], [ 39, Rationals ], [ 39, Rationals ] ]
# But found:
[ [ 1, Rationals ], [ 1, Rationals ], [ 6, Rationals, 3, [ 2, 2, 0 ] ], 
  [ 6, Rationals, 3, [ 2, 2, 0 ] ], [ 13/2, Rationals, 6, [ 2, 5, 0 ] ], 
  [ 13/2, Rationals, 6, [ 2, 5, 0 ] ], 
  [ 8, NF(13,[ 1, 3, 9 ]), 3, [ 2, 2, 0 ] ], 
  [ 8, NF(13,[ 1, 3, 9 ]), 3, [ 2, 2, 0 ] ], [ 13, Rationals, 3, [ 2, 2, 0 ] ]
    , [ 13, Rationals, 3, [ 2, 2, 0 ] ], 
  [ 13, NF(8,[ 1, 3 ]), 3, [ 2, 2, 0 ] ], 
  [ 13, NF(8,[ 1, 3 ]), 3, [ 2, 2, 0 ] ], [ 27/2, Rationals, 3, [ 2, 2, 0 ] ],
  [ 27/2, Rationals, 3, [ 2, 2, 0 ] ], [ 39/2, Rationals, 3, [ 2, 2, 0 ] ], 
  [ 39/2, Rationals, 3, [ 2, 2, 0 ] ] ]
########

Any ideas?

drallenherman commented 4 years ago

@drallenherman @angeldelriomateos now that's very interesting. We are able to test manual examples, and here what's happening: they all pass in the GAP development version, however they fail in GAP release branches:

Click on the links and scroll to the bottom to see diffs. For example, what we have in stable-4.11 with fractional dimensions(!):

########> Diff in /home/travis/build/gap-packages/wedderga/gaproot/pkg/wedderg\
a/tst/wedderga07.tst:162
# Input is:
SimpleComponentByCharacterDescent(Rationals,G,8);
# Expected output:
[ 21/2, GaussianRationals, 12, [ 2, 5, 9 ] ]
# But found:
[ 21/2, GaussianRationals, 12, [ 2, 5, 3 ] ]
########
########> Diff in /home/travis/build/gap-packages/wedderga/gaproot/pkg/wedderg\
a/tst/wedderga07.tst:186
# Input is:
WedderburnDecompositionByCharacterDescent(Rationals,G);
# Expected output:
[ [ 1, Rationals ], [ 1, Rationals ], [ 12, Rationals ], [ 12, Rationals ],
[ 13, Rationals ], [ 13, Rationals ], [ 16, NF(13,[ 1, 3, 9 ]) ],
[ 16, NF(13,[ 1, 3, 9 ]) ], [ 26, Rationals ], [ 26, Rationals ],
[ 26, NF(8,[ 1, 3 ]) ], [ 26, NF(8,[ 1, 3 ]) ], [ 27, Rationals ],
[ 27, Rationals ], [ 39, Rationals ], [ 39, Rationals ] ]
# But found:
[ [ 1, Rationals ], [ 1, Rationals ], [ 6, Rationals, 3, [ 2, 2, 0 ] ], 
  [ 6, Rationals, 3, [ 2, 2, 0 ] ], [ 13/2, Rationals, 6, [ 2, 5, 0 ] ], 
  [ 13/2, Rationals, 6, [ 2, 5, 0 ] ], 
  [ 8, NF(13,[ 1, 3, 9 ]), 3, [ 2, 2, 0 ] ], 
  [ 8, NF(13,[ 1, 3, 9 ]), 3, [ 2, 2, 0 ] ], [ 13, Rationals, 3, [ 2, 2, 0 ] ]
    , [ 13, Rationals, 3, [ 2, 2, 0 ] ], 
  [ 13, NF(8,[ 1, 3 ]), 3, [ 2, 2, 0 ] ], 
  [ 13, NF(8,[ 1, 3 ]), 3, [ 2, 2, 0 ] ], [ 27/2, Rationals, 3, [ 2, 2, 0 ] ],
  [ 27/2, Rationals, 3, [ 2, 2, 0 ] ], [ 39/2, Rationals, 3, [ 2, 2, 0 ] ], 
  [ 39/2, Rationals, 3, [ 2, 2, 0 ] ] ]
########

Any ideas?

1) is a randomization error, the algebras are isomorphic. This group is a permutation group so that can happen. WDByCD doesn't call the Warning message for fractional matrix algebras yet so that is something that we could add. 2) is caused by a change to GSOCA. It looks like we've brought back an old error again in the length 4 loop. Trivial factor sets will not survive GSOCA when this is fixed.

olexandr-konovalov commented 4 years ago

@drallenherman for the manual, unless there is a need in the very these examples, I can change massage examples until they will be the same in all versions then. If you have a fix for "an error in length 4 loop" you can not try to submit a pull request following https://github.com/gap-packages/wedderga/wiki - I am here to assist.

drallenherman commented 4 years ago

@drallenherman @angeldelriomateos now that's very interesting. We are able to test manual examples, and here what's happening: they all pass in the GAP development version, however they fail in GAP release branches:

Click on the links and scroll to the bottom to see diffs. For example, what we have in stable-4.11 with fractional dimensions(!):

########> Diff in /home/travis/build/gap-packages/wedderga/gaproot/pkg/wedderg\
a/tst/wedderga07.tst:162
# Input is:
SimpleComponentByCharacterDescent(Rationals,G,8);
# Expected output:
[ 21/2, GaussianRationals, 12, [ 2, 5, 9 ] ]
# But found:
[ 21/2, GaussianRationals, 12, [ 2, 5, 3 ] ]
########
########> Diff in /home/travis/build/gap-packages/wedderga/gaproot/pkg/wedderg\
a/tst/wedderga07.tst:186
# Input is:
WedderburnDecompositionByCharacterDescent(Rationals,G);
# Expected output:
[ [ 1, Rationals ], [ 1, Rationals ], [ 12, Rationals ], [ 12, Rationals ],
[ 13, Rationals ], [ 13, Rationals ], [ 16, NF(13,[ 1, 3, 9 ]) ],
[ 16, NF(13,[ 1, 3, 9 ]) ], [ 26, Rationals ], [ 26, Rationals ],
[ 26, NF(8,[ 1, 3 ]) ], [ 26, NF(8,[ 1, 3 ]) ], [ 27, Rationals ],
[ 27, Rationals ], [ 39, Rationals ], [ 39, Rationals ] ]
# But found:
[ [ 1, Rationals ], [ 1, Rationals ], [ 6, Rationals, 3, [ 2, 2, 0 ] ], 
  [ 6, Rationals, 3, [ 2, 2, 0 ] ], [ 13/2, Rationals, 6, [ 2, 5, 0 ] ], 
  [ 13/2, Rationals, 6, [ 2, 5, 0 ] ], 
  [ 8, NF(13,[ 1, 3, 9 ]), 3, [ 2, 2, 0 ] ], 
  [ 8, NF(13,[ 1, 3, 9 ]), 3, [ 2, 2, 0 ] ], [ 13, Rationals, 3, [ 2, 2, 0 ] ]
    , [ 13, Rationals, 3, [ 2, 2, 0 ] ], 
  [ 13, NF(8,[ 1, 3 ]), 3, [ 2, 2, 0 ] ], 
  [ 13, NF(8,[ 1, 3 ]), 3, [ 2, 2, 0 ] ], [ 27/2, Rationals, 3, [ 2, 2, 0 ] ],
  [ 27/2, Rationals, 3, [ 2, 2, 0 ] ], [ 39/2, Rationals, 3, [ 2, 2, 0 ] ], 
  [ 39/2, Rationals, 3, [ 2, 2, 0 ] ] ]
########

Any ideas?

  1. is a randomization error, the algebras are isomorphic. This group is a permutation group so that can happen. WDByCD doesn't call the Warning message for fractional matrix algebras yet so that is something that we could add.
  2. is caused by a change to GSOCA. It looks like we've brought back an old error again in the length 4 loop. Trivial factor sets will not survive GSOCA when this is fixed.

for 2) it was caused by a misplaced fi; in GSOCA. This will be fixed now.

drallenherman commented 4 years ago

@drallenherman for the manual, unless there is a need in the very these examples, I can change massage examples until they will be the same in all versions then. If you have a fix for "an error in length 4 loop" you can not try to submit a pull request following https://github.com/gap-packages/wedderga/wiki - I am here to assist.

I've fixed the length 4 loop problem by repositioning one fi;, this example is now working correctly on my machine. I can commit and push this but I will look at your wiki first just in case. The new examples are designed to cover the new functions so yes I think we want to have them. And thanks for fixing all of my GAPDoc typos.

angeldelriomateos commented 4 years ago

I checked the bug mentioned by Allen about two F2s, one of them should be F1. However in my version it is in line 2496, not in the line mentioned by Allen. Allen, why don't you fix it and do commit and push of both this and the fi fixing in GSOCA?

olexandr-konovalov commented 4 years ago

@drallenherman I really suggest to follow my Wiki a create a pull request to divalg branch - what you have done is a direct commit instead, and the diff show again whitespace changes.

olexandr-konovalov commented 4 years ago

I suggest the following fix for now: I will reset the divalg branch to and then each of you will have to reset your local clones. Then we will try with @drallenherman to submit the fix properly.

olexandr-konovalov commented 4 years ago

@drallenherman @angeldelriomateos aha, wait - I panicked because it stuck me that the fix went into divalg branch. Aha, never mind then - for now this is fine. But I will now edit your last commit to remove whitespace changes.

olexandr-konovalov commented 4 years ago

@drallenherman thanks for the fix - I have edited it and force-pushed in 8fbc72e0946d4ee46c0ae2cd6f90c2aaac6ae5d8. So now it only has missing fi added. Let's see how tests will behave now.

Note that because it is force-pushed, if you will be editing the fix-gapdoc branch further, you will have to reset your local checkout of this branch (e.g. doing git reset HEAD~1, assuming that the missing fi is added in the last commit - if the history diverged further, there is a more radical way to reset it).

olexandr-konovalov commented 4 years ago

@angeldelriomateos of course, line numbers may drift over revisions. Since F2->F1 issue is not related to this PR, I will leave it to one of you as a simple example of the pull request workflow!

drallenherman commented 4 years ago

@drallenherman thanks for the fix - I have edited it and force-pushed in 8fbc72e. So now it only has missing fi added. Let's see how tests will behave now.

Note that because it is force-pushed, if you will be editing the fix-gapdoc branch further, you will have to reset your local checkout of this branch (e.g. doing git reset HEAD~1, assuming that the missing fi is added in the last commit - if the history diverged further, there is a more radical way to reset it).

All right. I will change to that branch. Is the checkout step done automatically with Github Desktop? I don't see anything with "checkout" on it that I can click on.
I didn't make the change to F1 in ReducingCyclotomicAlgebra yet. Only saw Angel's message after I pushed.

olexandr-konovalov commented 4 years ago

Force-pushed again. I will be fiddling with this branch for a while.

Is the checkout step done automatically with Github Desktop? I don't see anything with "checkout" on it that I can click on.

I have installed Github Desktop and will try to figure out. Interestingly in my Git journey, I initially used GUI (SourceTree), then started to use it only to explore the history, and then completely moved to command line.

I didn't make the change to F1 in ReducingCyclotomicAlgebra yet. Only saw Angel's message after I pushed.

Fine, I suggest to do it separately.

angeldelriomateos commented 4 years ago

I will commit the change F2->F1. I think that it will go to div-alg-bug because it is were my computer is forwarding everything. You tell me if I have to change this.

olexandr-konovalov commented 4 years ago

@angeldelriomateos div-alg-bug does not exist any more! You need to follow https://github.com/gap-packages/wedderga/wiki for the current workflow.

codecov[bot] commented 4 years ago

Codecov Report

Merging #59 into divalg will increase coverage by 2.07%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           divalg      #59      +/-   ##
==========================================
+ Coverage   75.30%   77.37%   +2.07%     
==========================================
  Files          11       11              
  Lines        5313     5313              
==========================================
+ Hits         4001     4111     +110     
+ Misses       1312     1202     -110
Impacted Files Coverage Δ
lib/main.gi 81.43% <0.00%> (+0.61%) :arrow_up:
lib/div-alg.gi 74.19% <0.00%> (+5.66%) :arrow_up:
olexandr-konovalov commented 4 years ago

OK, we are back to square one here, but now Travis and Codecov work properly. Regarding tests which depend on randomness, I could try to find examples which are more reproducible - but we may still want to fix two problems (fi and F2->F1) elsewhere, and then rebase this PR on top of those fixes and test again.

olexandr-konovalov commented 4 years ago

I have now rebased this PR on top of F2->F1 fix, and PR #60 - so you can now see that PR #59 contains only one commit, which updates manual examples and their tests. This is an illustration of techniques which you can do making pull requests.

@drallenherman if you have a fix for fi, and will try to submit it in a pull request made to divalg branch, that would be great.

olexandr-konovalov commented 4 years ago

Rebased on #61 (fix fi) - let's see the effect on tests after this.

olexandr-konovalov commented 4 years ago

Nice! With the "fi-fix", all examples pass in GAP 4.10, otherwise only one diff remains:

SimpleComponentByCharacterDescent(Rationals,G,8);
# Expected output:
[ 21/2, GaussianRationals, 12, [ 2, 5, 9 ] ]
# But found:
[ 21/2, GaussianRationals, 12, [ 2, 5, 3 ] ]
########

The full example is

gap> G:=PSU(3,3);
<permutation group of size 6048 with 2 generators>
gap> SimpleComponentByCharacterDescent(Rationals,G,8);
[ 21/2, GaussianRationals, 12, [ 2, 5, 9 ] ]
gap> SchurIndex(last);
1

Any ideas how to change that example to be more reproducible?

drallenherman commented 4 years ago

In the PSU (3,3) example it is OK to change the 9 to a 3 the resultING algebras are isomorphic.

angeldelriomateos commented 4 years ago

In the PSU (3,3) example it is OK to change the 9 to a 3 the resultING algebras are isomorphic.

Indeed, they are isomorphic. Actually, if we implement my proposal of modifying WedderburnDecompositionInfo the output whould be [21, GaussianRationals] which is better because this way we will get rid of most matrix algebras of fractional size. I am going to start a pull request with that.

angeldelriomateos commented 4 years ago

Before that I explain here the strategy for this new pull request. The idea is to modify the function SimpleAlgebraInfoByData to explode the functionality of the new function GlobalSplittingOfCyclotomicAlgebra (by the way I think that this is a missleading name because it looks that the output should be a splitting algebra while it is not) and the SchurIndex function which was alredy before. I have two versions in mind. The first one only uses GSOCA. The second one will use both. They might slow down the calculations but I think that this won't be significant for the first one. I must say that I also discovered some issue with the second one. So I am going to go ahead first with the first one.

olexandr-konovalov commented 4 years ago

Swapping 9 and 3 makes is pass in GAP master and stable-4.11, but fail in 4.9 and 4.10 (see https://travis-ci.org/github/gap-packages/wedderga/builds/673593975). We can of course abandon testing with old GAP releases, and say that Wedderga requires GAP 4.11 or newer, but that may be too radical - perhaps an example may be fixed at a lower cost. It depends what are you demonstrating by this example. If you only want to show that it works, one could replace 8 by some other index, where there will be no random fluctuations. Or if you want to show that SchurIndex works and is 1, you can use double semicolon to suppress the output of SimpleComponentByCharacterDescent, or even squeeze it like this:

gap> G:=PSU(3,3);
<permutation group of size 6048 with 2 generators>
gap> SchurIndex(SimpleComponentByCharacterDescent(Rationals,G,8));
1

Or, do this:

gap> G:=PSU(3,3);
<permutation group of size 6048 with 2 generators>
gap> sc := SimpleComponentByCharacterDescent(Rationals,G,8);;
gap> sc{[1..3]}; # the 4th is [ 2, 5, 3 ] or [ 2, 5, 9 ] 
[ 21/2, GaussianRationals, 12 ]
gap> SchurIndex(sc);
1
olexandr-konovalov commented 4 years ago

@angeldelriomateos thanks, please go ahead with the intended PR! Just to say, that to start a new discussion of a new change without a pull request yet, the best way is to create a new issue at https://github.com/gap-packages/wedderga/issues - if you comment under a pull request dedicated to another topic, it will go below the radar as soon as this pull request will be merged, and it will be hard to find it afterwards.

drallenherman commented 4 years ago

Before that I explain here the strategy for this new pull request. The idea is to modify the function SimpleAlgebraInfoByData to explode the functionality of the new function GlobalSplittingOfCyclotomicAlgebra (by the way I think that this is a missleading name because it looks that the output should be a splitting algebra while it is not) and the SchurIndex function which was alredy before. I have two versions in mind. The first one only uses GSOCA. The second one will use both. They might slow down the calculations but I think that this won't be significant for the first one. I must say that I also discovered some issue with the second one. So I am going to go ahead first with the first one.

My intention in calling this method "Global Splitting" is it is the acting of splitting a crossed product algebra as much as it can be split over a Global field (i.e. an algebraic number field), as opposed to the Local index algorithms that rely on the arithmetic in a crossed product over a Local field. We just change the basis by a simple rescaling by roots of unity until we get a tensor decomposition A = M_n(F) x B and then reduce to B. The Global means it is as much splitting as we can do over the Global field F, it doesn't mean the result has to be split.

olexandr-konovalov commented 4 years ago

It's tempting to do 

gap> G:=PSU(3,3);
<permutation group of size 6048 with 2 generators>
gap> sc := SimpleComponentByCharacterDescent(Rationals,G,8);;
gap> sc{[1..3]}; # the 4th entry is [ 2, 5, 3 ] or [ 2, 5, 9 ]
[ 21/2, GaussianRationals, 12 ]
gap> SchurIndex(sc);
1

at least for now, and merge this PR - it will add more tests due to new manual examples and will allow to better test further modifications in divalg.

olexandr-konovalov commented 4 years ago

One more random trouble testing with GAP from stable-4.9: in

gap> G:=SmallGroup(160,207);
<pc group of size 160 with 6 generators>
gap> W:=WedderburnDecompositionInfo(GroupRing(Rationals,G));;
gap> W[20];
[ 1, Rationals, 20, [ [ 2, 11, 0 ], [ 4, 3, 0 ] ], [ [ 0 ] ] ]
gap> GlobalSplittingOfCyclotomicAlgebra(W[20]);
[ 8, Rationals ]

we have

########> Diff in /home/travis/build/gap-packages/wedderga/gaproot/pkg/wedderg\
a/tst/wedderga07.tst:143
# Input is:
W[20];
# Expected output:
[ 1, Rationals, 20, [ [ 2, 11, 0 ], [ 4, 3, 0 ] ], [ [ 0 ] ] ]
# But found:
[ 2, Rationals, 10, [ 4, 3, 0 ] ]
########
drallenherman commented 4 years ago

One more random trouble testing with GAP from stable-4.9: in

gap> G:=SmallGroup(160,207);
<pc group of size 160 with 6 generators>
gap> W:=WedderburnDecompositionInfo(GroupRing(Rationals,G));;
gap> W[20];
[ 1, Rationals, 20, [ [ 2, 11, 0 ], [ 4, 3, 0 ] ], [ [ 0 ] ] ]
gap> GlobalSplittingOfCyclotomicAlgebra(W[20]);
[ 8, Rationals ]

we have

########> Diff in /home/travis/build/gap-packages/wedderga/gaproot/pkg/wedderg\
a/tst/wedderga07.tst:143
# Input is:
W[20];
# Expected output:
[ 1, Rationals, 20, [ [ 2, 11, 0 ], [ 4, 3, 0 ] ], [ [ 0 ] ] ]
# But found:
[ 2, Rationals, 10, [ 4, 3, 0 ] ]
########

We need to change the example now. Forget the group. Change the input to gap> A;= [ 1, Rationals, 20, [ [ 2, 11, 0 ], [ 4, 3, 0 ] ], [ [ 0 ] ] ]; and test with gap> GlobalSplittingOfCyclotomicAlgebra(A); [ 8, Rationals ]

drallenherman commented 4 years ago

It's tempting to do

gap> G:=PSU(3,3);
<permutation group of size 6048 with 2 generators>
gap> sc := SimpleComponentByCharacterDescent(Rationals,G,8);;
gap> sc{[1..3]}; # the 4th entry is [ 2, 5, 3 ] or [ 2, 5, 9 ]
[ 21/2, GaussianRationals, 12 ]
gap> SchurIndex(sc);
1

at least for now, and merge this PR - it will add more tests due to new manual examples and will allow to better test further modifications in divalg.

We will need to do this in any cases. SimpleComponentByCharacterDescent does not use WDI so this won't be impacted by changes to WDI.

olexandr-konovalov commented 4 years ago

@drallenherman thanks, the latest fix of the manual example resolved all problems!

Now divalg branch is updated, and you and @drallenherman need to update your local checkouts of this branch with

git checkout divalg
git pull origin divalg

After that, you can check with git log that you see Fix GAPDoc syntax in the documentation as the latest commit.