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

Adjusted SchurIndexByCharacter to use character descent (2nd attempt) #67

Closed drallenherman closed 4 years ago

drallenherman commented 4 years ago

This is a better attempt of PR #66.

Closes issue #65

olexandr-konovalov commented 4 years ago

This had master as a base branch - changed it to divalg, but it's still incorrect. First, please:

We will then try to edit this PR using interactive rebase in git command line to get rid of all unwanted commits.

codecov[bot] commented 4 years ago

Codecov Report

Merging #67 into divalg will increase coverage by 0.03%. The diff coverage is 91.30%.

@@            Coverage Diff             @@
##           divalg      #67      +/-   ##
==========================================
+ Coverage   79.35%   79.38%   +0.03%     
==========================================
  Files          11       11              
  Lines        5313     5321       +8     
==========================================
+ Hits         4216     4224       +8     
  Misses       1097     1097              
Impacted Files Coverage Δ
lib/div-alg.gi 80.05% <91.30%> (+0.03%) :arrow_up:
lib/main.gi 81.51% <0.00%> (+0.07%) :arrow_up:
olexandr-konovalov commented 4 years ago

The new example triggered another diff in tests:

testing: /Users/alexk/.gap/pkg/wedderga/tst/wedderga07.tst
########> Diff in /Users/alexk/.gap/pkg/wedderga/tst/wedderga07.tst:389
# Input is:
b:=Basis(last);
# Expected output:
CanonicalBasis( <algebra-with-one of dimension 4 over NF(24,[ 1, 11 ])> )
# But found:
Basis( <algebra-with-one of dimension 4 over NF(24,[ 1, 11 ])>, ... )
########

I have a fixup commit - shall I push it?

drallenherman commented 4 years ago

I discovered why J2 was performing well on my computer. I had done J1 already. After J1 the Schur indices of J2 characters up to Irr(G)[20] run almost immediately. J1 has order 175560 and Irr(G)[11] runs in just over 2 minutes. So these are a bit slow.

For G:=AtlasGroup("2.A8"); wedderga computes SchurIndexByCharacter(Rationals,G,15)=2 in under 4 seconds from a cold start. The group order is 40320 and this is its unique character of degree 48. So it is safer to use this example instead of the J2 one.

drallenherman commented 4 years ago

The new example triggered another diff in tests:

testing: /Users/alexk/.gap/pkg/wedderga/tst/wedderga07.tst
########> Diff in /Users/alexk/.gap/pkg/wedderga/tst/wedderga07.tst:389
# Input is:
b:=Basis(last);
# Expected output:
CanonicalBasis( <algebra-with-one of dimension 4 over NF(24,[ 1, 11 ])> )
# But found:
Basis( <algebra-with-one of dimension 4 over NF(24,[ 1, 11 ])>, ... )
########

I have a fixup commit - shall I push it?

Yes. I don't think this will make much difference, except perhaps it won't be in agreement with tests running on other versions of GAP?

olexandr-konovalov commented 4 years ago

@drallenherman

Yes. I don't think this will make much difference, except perhaps it won't be in agreement with tests running on other versions of GAP?

No, it happened in all 5 combinations. Some effect of loading methods perhaps, triggered by using AtlasRep package. I've pushed a fixup commit.

olexandr-konovalov commented 4 years ago

We can keep J2 example in the manual, and have J1 and anything else in tst/div-alg.tst where you should be easily able to add them yourself. 2 minutes is OK.

drallenherman commented 4 years ago

We can keep J2 example in the manual, and have J1 and anything else in tst/div-alg.tst where you should be easily able to add them yourself. 2 minutes is OK.

Ok. the 2.A8 example is one of the only accessible ones that gives a nontrivial Schur index in a reasonable amount of time that's why I like it. J2 and J1 return Schur index 1 for all characters. I will take care of this when I review documentation. Important now to get Angel's version merged and stable with the rebased divalg.

olexandr-konovalov commented 4 years ago

@drallenherman @angeldelriomateos rebased this PR, now reduced to 5 commits, will merge after the round of tests.

olexandr-konovalov commented 4 years ago

Merged. @drallenherman now do this:

git checkout divalg
git pull origin divalg

to update your divalg branch, and then

git branch -D pr/65

to delete pr/65 branch which you don't need any more.

drallenherman commented 4 years ago

Merged. @drallenherman now do this:

git checkout divalg
git pull origin divalg

to update your divalg branch, and then

git branch -D pr/65

to delete pr/65 branch which you don't need any more.

ok. done. I think successfully. Thank you!