olexandr-konovalov commented 4 years ago

@drallenherman and @angeldelriomateos are now working on modernising the div-alg code. We are consolidating all work in this branch, and will from now on work on submitting small individual pull requests to this branch. This will give us opportunity to better review and test that changes work, and let the code stabilise here before we merge it into master and publish a new release. Should be also easier to resolve conflicts.

I have just submitted the first of the PRs to this branch in #59, and currently investigating why tests of manual examples fail in some older versions of GAP. I used that opportunity to test and document the contributing process - please see https://github.com/gap-packages/wedderga/wiki for instructions.

What would be useful to do before merging this:

[ ] add more tests to improve code coverage
[ ] remove commented out code which is no longer needed (if you need to preserve commented out code, I've rely on version control to keep its previous version; commenting out a block of code and writing a new code below makes diffs unclear, whereas editing IN the code makes changes more obvious)
[ ] improve code formatting: not all code is properly indented. This should be done at the end, in a separate commit, not to be mixed with actual changes
[ ] clean up use of GAPDoc elements in the documentation (<A>, <C> and <M>) instead of <C> in most places).

codecov[bot] commented 4 years ago

Codecov Report

Merging #58 into master will increase coverage by 2.96%. The diff coverage is 92.40%.

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
+ Coverage   80.49%   83.45%   +2.96%     
==========================================
  Files          11       11              
  Lines        4650     5193     +543     
==========================================
+ Hits         3743     4334     +591     
+ Misses        907      859      -48

Impacted Files	Coverage Δ
lib/main.gi	`81.03% <27.27%> (+0.20%)`	:arrow_up:
lib/div-alg.gi	`93.12% <92.95%> (+7.22%)`	:arrow_up:
lib/div-alg.gd	`100.00% <100.00%> (ø)`

angeldelriomateos commented 4 years ago

I pushed the F2->F1 replacement. Why I can not see that commitment?

olexandr-konovalov commented 4 years ago

@angeldelriomateos because you pushed it to some other place, which is no longer connected to this pull request. You need to update your setup now following https://github.com/gap-packages/wedderga/wiki - that's a more robust workflow, which would be good to adopt now. But if you wish, I can cherry-pick your commit from https://github.com/alex-konovalov/wedderga/commit/83cf230a851cd59022f343eb9503d0f040538f54 myself for now.

angeldelriomateos commented 4 years ago

Ok. I am following the instructions cloning and everything else. So far I cloned the repository made the commitment and then try to push. The commit seems to be done but the push was rejected. I think that I have to continue with the instructions: fork,....

angeldelriomateos commented 4 years ago

I understand that after cloning using git clone https://github.com/gap-packages/wedderga.git I have to fork. For that, following the instruction I navigated to wedderga/divalg and clicked on fork. A message showed up that I alredy forked wedderga and mention angeldelriomateos/wedderga. I went there but the files there are old. What should I do then?

olexandr-konovalov commented 4 years ago

@angeldelriomateos it does not matter that the fork is old - it matters that it exists so you can push branches to it. So go the next step, you're on the right way!

angeldelriomateos commented 4 years ago

It seems that I managed to do the F2->F1 change in divalg. I guess that Allen fixing is not commited yet because I cannot see it in the log.

olexandr-konovalov commented 4 years ago

Thanks, you did that, indeed! Not completely following the pull request workflow, but for small obviously correct changes that's tolerable (although, one should never say "obviously correct" without testing - that's one of the nicest things for using pull requests; the other is ability to review the code by others BEFORE it is merged).

angeldelriomateos commented 4 years ago

Thanks, you did that, indeed! Not completely following the pull request workflow, but for small obviously correct changes that's tolerable (although, one should never say "obviously correct" without testing - that's one of the nicest things for using pull requests; the other is ability to review the code by others BEFORE it is merged).

I see. Instead of git commit -a I should have done git commit angeldelriomateos. Then I should follow the instructions for pull request. Learning one by one

olexandr-konovalov commented 4 years ago

More precisely, you would have to do git checkout -b fix-F2-F1, then git commit -a and then git push angeldelriomateos. Getting there!

angeldelriomateos commented 4 years ago

Still we are waiting for Allen push on fi. After that there is an improvement that I think we should do, namely explode GSOCA to improve the output of WeddderburnDecompositionInfo. For example, at this moment the wedderburn decomposition of QS_3 looks like: [ [ 1, Rationals ], [ 1, Rationals ], [ 1, Rationals, 3, [ 2, 2, 0 ] ] ] With that improvement it would look as follow: [ [ 1, Rationals ], [ 1, Rationals ], [ 2, Rationals ] ] In theory also WedderburnDecomposition might be improved but this might require a harder work.

angeldelriomateos commented 4 years ago

I found out where the fi bug is, so I am going to commit it myself.

angeldelriomateos commented 4 years ago

More precisely, you would have to do git checkout -b fix-F2-F1, then git commit -a and then git push angeldelriomateos. Getting there!

I followed this instructions and the last git push failed. I think that I didn't settle properly my username. Indeed, when I was doing git remote I wrote: git remote add angeldelriomateos https://github.com/your-username/wedderga.git I think that I should have writen git remote add angeldelriomateos https://github.com/angeldelriomateos/wedderga.git If I do that now it says that angeldelriomateos have already exists I guess that I invent a new nick name but this is probably a bad practice. Can I go backward?

olexandr-konovalov commented 4 years ago

@angeldelriomateos call this:

git remote set-url angeldelriomateos https://github.com/angeldelriomateos/wedderga.git

and then

git remote -v

to check that it had effect.

drallenherman commented 4 years ago

Still we are waiting for Allen push on fi. After that there is an improvement that I think we should do, namely explode GSOCA to improve the output of WeddderburnDecompositionInfo. For example, at this moment the wedderburn decomposition of QS_3 looks like: [ [ 1, Rationals ], [ 1, Rationals ], [ 1, Rationals, 3, [ 2, 2, 0 ] ] ] With that improvement it would look as follow: [ [ 1, Rationals ], [ 1, Rationals ], [ 2, Rationals ] ] In theory also WedderburnDecomposition might be improved but this might require a harder work.

It seems I missed a lot. I had to go help Gwen make our Easter bread! I might have it all to myself this year :-)
Yes I was hoping we could apply GlobalSplitting to WDInfo so that only interesting simple components are produced. But I think we need to discuss this with Sugandha and Gurmeet. It seems one test is still failing? Alex said moving the fi: position might not be the only issue. But that was back on #57.

olexandr-konovalov commented 4 years ago

It seems @angeldelriomateos's #61 tests well - I will shortly merge it, and then rebase #59 on top of it - then we will see whether the fi problem will fix the tests...

drallenherman commented 4 years ago

Testing some more ideas. Most of the time needed for local indices of the harder groups order 1920 is used by DefiningGroup at the g1:=IsomorphismSpecialPcGroup Step. This takes almost 270 seconds, after which it takes 10 for the character table. Replacing this with g1:=IsomorphismPermGroup it took 1.3 seconds but 13 to get the character table.

We should change this to use PermGroup in DefiningGroup and DefiningGroupAndCharacter.

angeldelriomateos commented 4 years ago

Testing some more ideas. Most of the time needed for local indices of the harder groups order 1920 is used by DefiningGroup at the g1:=IsomorphismSpecialPcGroup Step. This takes almost 270 seconds, after which it takes 10 for the character table. Replacing this with g1:=IsomorphismPermGroup it took 1.3 seconds but 13 to get the character table.

We should change this to use PermGroup in DefiningGroup and DefiningGroupAndCharacter.

I think that you should go ahead with this. This morning with the help of Alex I manage to update my version with Allen fixing in Galois descents and include my fixing on combination of SimpleAlgebraInfoWithData in some local index calculations and everything seems to be working smooth. The last improvement mentioned by Allen will be great if it shorten time calculation. I am very happy on how everything seems to be working now and this reduction in time calculation is welcomed.

olexandr-konovalov commented 4 years ago

@drallenherman @angeldelriomateos Hooray, with the new tests this PR now tests just above 90% of diffs in the code!

olexandr-konovalov commented 4 years ago

@drallenherman @angeldelriomateos is there anything left before we merge this and make a new release 4.10.0 or maybe even make it version 5.0.0 ?

angeldelriomateos commented 4 years ago

I don't have anything new to add except maybe a library of decompositions. If Allen agrees one could merge everything and release a new version. Whether it should be 4.10.0 or 5.0.0 you know better what is the best alternative.

olexandr-konovalov commented 4 years ago

@angeldelriomateos the library would be is a separate project, which should rather be done in a separate branch and submitted as a PR to master after merging divalg. Thus, merging this PR, making a new release, and then adding a library on top of the stable version of the package would be ideal.

drallenherman commented 4 years ago

I don't have anything new to add except maybe a library of decompositions. If Allen agrees one could merge everything and release a new version. Whether it should be 4.10.0 or 5.0.0 you know better what is the best alternative.

I don't have anything more to add. I think we are ready to release this now. I'm ok with calling it 4.10.0. It doesn't make sense to call it 4.9.6 because we've added new features, but the main commands are still the same.

olexandr-konovalov commented 4 years ago

Ok - then I will make a couple of edits in commit messages and then merge it.

olexandr-konovalov commented 4 years ago

Merged! thanks @angeldelriomateos and @drallenherman !

olexandr-konovalov commented 4 years ago

@drallenherman @angeldelriomateos version 4.10.0 now at https://gap-packages.github.io/wedderga/. It should be picked up by GAP package update system tomorrow for further testing.

gap-packages / wedderga

Overhaul of div-alg code #58

Codecov Report