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

Bugfix and speedup in KillingCocycle #78

Closed olexandr-konovalov closed 4 years ago

olexandr-konovalov commented 4 years ago

Bug corrects a wrong rewritting of the input. Speed up in checking whether a cyclic algebra (K/F,a) is split because a is the norm of a root of 1.

This re-submits changes from #77.

codecov[bot] commented 4 years ago

Codecov Report

Merging #78 into divalg will increase coverage by 0.19%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           divalg      #78      +/-   ##
==========================================
+ Coverage   82.30%   82.50%   +0.19%     
==========================================
  Files          11       11              
  Lines        5193     5189       -4     
==========================================
+ Hits         4274     4281       +7     
+ Misses        919      908      -11
Impacted Files Coverage Δ
lib/div-alg.gi 90.17% <100.00%> (+0.63%) :arrow_up:
olexandr-konovalov commented 4 years ago

@angeldelriomateos now tests pass here, fail in divalg, and have 100% diffs coverage. If all changes at https://github.com/gap-packages/wedderga/pull/78/files look ok to you, I will merge this and close #79.

drallenherman commented 4 years ago

I am back now. I have a new version of DecomposeCyclotomicAlgebra composed so I will start a new pull request for that tomorrow.

I also worked on a second KillingCocycle command because the new DecomposeCA will use it. When I got back I tested it against Angel's new one in pr/79. The results are very similar - but mine gets more zeroes in A[5] in some cases.

Angel, in the creation of "as" in your KillingCocycle, the j is running over [1..k]. I think it should run over Difference([1..k],[i]) because the case i=j is not relevant, and the solutions the i adds to the list are preventing some 0's from being created. Look at the faithful component of SG(96,114). My KillingCocycle produces a 0 in A[5] for this one. Every case I have noticed where my result differs from yours is because of this issue.

drallenherman commented 4 years ago

I am back now. I have a new version of DecomposeCyclotomicAlgebra composed so I will start a new pull request for that tomorrow.

I also worked on a second KillingCocycle command because the new DecomposeCA will use it. When I got back I tested it against Angel's new one in pr/79. The results are very similar - but mine gets more zeroes in A[5] in some cases.

Angel, in the creation of "as" in your KillingCocycle, the j is running over [1..k]. I think it should run over Difference([1..k],[i]) because the case i=j is not relevant, and the solutions the i adds to the list are preventing some 0's from being created. Look at the faithful component of SG(96,114). My KillingCocycle produces a 0 in A[5] for this one. Every case I have noticed where my result differs from yours is because of this issue.

Oops! I just realized my div-alg had been pre-adjusted so that the old KC was not being applied in WDInfo and that meant I was not comparing my KC to yours. With or without the change I'm suggesting above, it now seems both KCs are producing the same output.

angeldelriomateos commented 4 years ago

I am back now. I have a new version of DecomposeCyclotomicAlgebra composed so I will start a new pull request for that tomorrow. I also worked on a second KillingCocycle command because the new DecomposeCA will use it. When I got back I tested it against Angel's new one in pr/79. The results are very similar - but mine gets more zeroes in A[5] in some cases. Angel, in the creation of "as" in your KillingCocycle, the j is running over [1..k]. I think it should run over Difference([1..k],[i]) because the case i=j is not relevant, and the solutions the i adds to the list are preventing some 0's from being created. Look at the faithful component of SG(96,114). My KillingCocycle produces a 0 in A[5] for this one. Every case I have noticed where my result differs from yours is because of this issue.

Oops! I just realized my div-alg had been pre-adjusted so that the old KC was not being applied in WDInfo and that meant I was not comparing my KC to yours. With or without the change I'm suggesting above, it now seems both KCs are producing the same output.

You are right that j=i is not relevant, and I gues that you are also right that some possible solutions could be excluded considering j=i. I was aware of the first but not of the second, so when I wrote the code I consider not necessary to exclude j=i. Somehow I am surprised that no difference comes up when j=i is excluded.

drallenherman commented 4 years ago

I am back now. I have a new version of DecomposeCyclotomicAlgebra composed so I will start a new pull request for that tomorrow. I also worked on a second KillingCocycle command because the new DecomposeCA will use it. When I got back I tested it against Angel's new one in pr/79. The results are very similar - but mine gets more zeroes in A[5] in some cases. Angel, in the creation of "as" in your KillingCocycle, the j is running over [1..k]. I think it should run over Difference([1..k],[i]) because the case i=j is not relevant, and the solutions the i adds to the list are preventing some 0's from being created. Look at the faithful component of SG(96,114). My KillingCocycle produces a 0 in A[5] for this one. Every case I have noticed where my result differs from yours is because of this issue.

Oops! I just realized my div-alg had been pre-adjusted so that the old KC was not being applied in WDInfo and that meant I was not comparing my KC to yours. With or without the change I'm suggesting above, it now seems both KCs are producing the same output.

You are right that j=i is not relevant, and I gues that you are also right that some possible solutions could be excluded considering j=i. I was aware of the first but not of the second, so when I wrote the code I consider not necessary to exclude j=i. Somehow I am surprised that no difference comes up when j=i is excluded.

All right. I've made this change in the new branch (pr/80).

olexandr-konovalov commented 4 years ago

@angeldelriomateos have you seen my comment above?

now tests pass here, fail in divalg, and have 100% diffs coverage. If all changes at https://github.com/gap-packages/wedderga/pull/78/files look ok to you, I will merge this and close #79.

olexandr-konovalov commented 4 years ago

Rebased on top of #81 merged into divalg. All is ready - waiting for @angeldelriomateos to confirm.