Overhaul of div-alg code (refactoring of PR 56) #57

olexandr-konovalov · 2020-04-09T12:55:27Z

This is refactoring of PR #56. Work in progress for @drallenherman and @angeldelriomateos to check.

codecov · 2020-04-09T13:00:18Z

Codecov Report

Merging #57 into master will decrease coverage by 5.69%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
- Coverage   80.49%   74.79%   -5.70%     
==========================================
  Files          11       11              
  Lines        4650     5309     +659     
==========================================
+ Hits         3743     3971     +228     
- Misses        907     1338     +431

Impacted Files	Coverage Δ
lib/div-alg.gi	`67.07% <ø> (-18.83%)`	⬇️
lib/div-alg.gd	`100.00% <100.00%> (ø)`
lib/crossed.gi	`73.77% <0.00%> (-0.15%)`	⬇️

olexandr-konovalov · 2020-04-09T14:50:38Z

I am comparing this with PR #56, to ensure that the result is identical:

$ diff -r ../wedderga/doc/ ../wedderga1/doc/
$ diff -r ../wedderga/lib/ ../wedderga1/lib/ 
```

drallenherman · 2020-04-09T16:27:28Z

This is refactoring of PR 56. Work in progress for @drallenherman and @angeldelriomateos to check.

Thank you Alexander, I've successfully rebased to the new branch.

olexandr-konovalov · 2020-04-09T17:47:43Z

@drallenherman I don't know what "rebased to the new branch" mean, because "rebase" usually means that you are placing some changes on top of another changes - for example, a pull request made on some older revision of the master branch is rebased to look like it is made on the most recent revision of the master branch. Perhaps you had something else in mind!

Anyhow, I have severely edit this branch again, reducing the number of commits from 20 to 6, so most likely you will have to do that operation again to sync your local copy. Sorry if I was unclear - when I say "work in progress to check", I usually mean "work in progress, do not checkout unless you really want, but watch it via the web interface". You can see https://github.com/gap-packages/wedderga/pull/57/commits on the level of individual commits, or at https://github.com/gap-packages/wedderga/pull/57/files to se cumulative diffs.

I have tried to untangle changes as best as I can, but then for some other changes I gave up and glued them together. Anyway, commits in this PR are not atomic (i.e. they contain unrelated changes), so I don't see the point, since anyway we will create a merge commit to accept all changes in once, and I can spend only a final amount of time of cleaning up the history.

So, I have now checked that this gives the same as PR #56, and I suggest to merge this into master, and start then again, following new rules in new PRs:

do not put unrelated changes in one commit (for example, bugfix, a new code, a documentation update for another code, and code reformatting).
do not put unrelated changes in one pull request.

Are you happy with this plan?

olexandr-konovalov · 2020-04-09T17:53:20Z

@drallenherman if you have already checked out this new branch, and want to reset it, assuming that origin points to this repo, and not to my fork (check with git remote -v), you will have to do this to reset it:

git fetch origin && git checkout div-alg-bug && git reset --hard origin/div-alg-bug

olexandr-konovalov · 2020-04-09T18:00:51Z

P.S. If you think that this is still a major rewrite and we should not merge this, we can go by some other way - you will be submitting new pull requests, but to this branch instead of a master. You have already practiced a bit, so next time it should be easier for you to make a pull request! Perhaps, the name of the branch should be changed to more informative though, since what you're doing is already beyond fixing a bug!

angeldelriomateos · 2020-04-09T18:04:07Z

Can I commit my fix of a bug on ReducingCyclotomicAlgebra?

olexandr-konovalov · 2020-04-09T18:05:55Z

@angeldelriomateos not yet, I'd rather try to agree with the plan!

angeldelriomateos · 2020-04-09T18:07:04Z

Ok. I wait instructions.... dear commander ;)

angeldelriomateos · 2020-04-09T18:13:38Z

Yes, this was one of the first fixing which is incorporated in the last push, and even before

olexandr-konovalov · 2020-04-09T18:19:25Z

Ok, so I will close #52 - because of indentation changes, it will be too tedious to get it merged, and then update this PR to resolve merge conflicts. Although, if that's a ready for the release bugfix, it may be still nice to get it merged...

olexandr-konovalov · 2020-04-09T18:21:35Z

Discovered that the documentation is broken - gap makedoc.g reports errors in XML code. So merging this could be a bit too early... I suggest to proceed with you submitting pull requests to a branch in wedderga's main repository.

drallenherman · 2020-04-09T18:31:15Z

@drallenherman I don't know what "rebased to the new branch" mean, because "rebase" usually means that you are placing some changes on top of another changes - for example, a pull request made on some older revision of the master branch is rebased to look like it is made on the most recent revision of the master branch. Perhaps you had something else in mind!

Anyhow, I have severely edit this branch again, reducing the number of commits from 20 to 6, so most likely you will have to do that operation again to sync your local copy. Sorry if I was unclear - when I say "work in progress to check", I usually mean "work in progress, do not checkout unless you really want, but watch it via the web interface". You can see https://github.com/gap-packages/wedderga/pull/57/commits on the level of individual commits, or at https://github.com/gap-packages/wedderga/pull/57/files to se cumulative diffs.

I have tried to untangle changes as best as I can, but then for some other changes I gave up and glued them together. Anyway, commits in this PR are not atomic (i.e. they contain unrelated changes), so I don't see the point, since anyway we will create a merge commit to accept all changes in once, and I can spend only a final amount of time of cleaning up the history.

So, I have now checked that this gives the same as PR #56, and I suggest to merge this into master, and start then again, following new rules in new PRs:

do not put unrelated changes in one commit (for example, bugfix, a new code, a documentation update for another code, and code reformatting).

do not put unrelated changes in one pull request.

Are you happy with this plan?

Yes, go ahead after Angel pushes his next bugfix. When I pulled this morning, there were a few merge conflicts but not on code so I just thought it must be the whitespace issue again. So I forced the merge and when I "rebased" to the new branch on github desktop nothing happened. Rebase to me only means I clicked on "rebase". Sorry for causing you so much stress, but actually those changes were related - I found those bugs in div-alg.gi because I was working on the documentation.

Is the whitespace issue fixed? I've left my Atom settings alone. I hope that isn't adding to your
stress.

We will temporarily use this version to distinguish the package loaded from this branch

This was done accidentally because of default Atom settings.

IsCyclotomicAlgebra(K,F) decides whether K is a cyclotomic extension of a NF. ReducingCyclotomicAlgebra(A) = Reduces the crossed product of a cyc. alg. as much as possible. FactoringCycAlg(A) outputs [A[1],B,C] where the crossed product of A is the tensor product of B and C.

Also extend LocalIndicesOfCyclotomicAlgebra mansection and save the file without trailing whitespaces.

…new commands

olexandr-konovalov · 2020-04-09T18:41:57Z

@angeldelriomateos please commit and push the bugfix to div-alg-bug in alex-konovalov/wedderga, don't make any other changes so far.

drallenherman · 2020-04-09T21:49:45Z

Hi Ángel, I've pulled the last version of dev-divalg that Alexander pushed. I was looking through RCA trying to understand why it makes GSOCA freeze on SG(160,137). Still not sure why that happens, but I think there is a typo in RCA: line 2489: I think the first F2 should be F1 Did you already fix these bugs and get GSOCA working smoothly with RCA? Allen

>> Ángel del Río ***@***.***> 04/09/20 12:13 PM >>>

Yes, this was one of the first fixing which is incorporated in the last push, and even before

…

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #57 (comment)

olexandr-konovalov · 2020-04-09T21:59:01Z

lib/div-alg.gi

+  y := Difference(v,x);
+  F1:=NF(con,List(y,j->A[4][j][2]));
+  F2:=NF(con,List(x,j->A[4][j][2]));
+  if IsCyclotomicExtension(F2,F) and IsCyclotomicExtension(F2,F) then    


@drallenherman you can comment immediately in the diffs for the pull request - so, should the 1st F2 be F1 ?

olexandr-konovalov · 2020-04-09T22:00:39Z

Speaking of bugs, please feel free to add new tests to catch them, e.g. as done in https://github.com/gap-packages/wedderga/pull/55/files

olexandr-konovalov · 2020-04-09T22:05:38Z

@drallenherman wrote

Yes, go ahead after Angel pushes his next bugfix. When I pulled this morning, there were a few merge conflicts but not on code so I just thought it must be the whitespace issue again. So I forced the merge and when I "rebased" to the new branch on github desktop nothing happened. Rebase to me only means I clicked on "rebase". Sorry for causing you so much stress, but actually those changes were related - I found those bugs in div-alg.gi because I was working on the documentation.

That explains perhaps a strange thing with changes made and then disappearing and then made again.

GitHub desktop seems to have "Pull origin with rebase" (https://help.github.com/en/desktop/contributing-to-projects/syncing-your-branch) - is that what you did?

Is the whitespace issue fixed? I've left my Atom settings alone. I hope that isn't adding to your
stress.

I haven't seen new problems, but only because you work with the same files for which trailing whitespaces are already removed. I don't know which settings your Atom has, so when you will edit another file, anything could happen, so please be careful.

olexandr-konovalov · 2020-04-09T22:39:45Z

P.S. I will put more detailed instructions on Wiki at https://github.com/gap-packages/wedderga/wiki tomorrow!

angeldelriomateos · 2020-04-10T08:43:33Z

I committed and pushed my fix bug in RCA causing problems with GSOCA.
It should not cause a problem now.
Hope to have it done properly this time. At least all looked correct.

olexandr-konovalov · 2020-04-10T09:03:37Z

@angeldelriomateos thanks, it seems your commit went smoothly. I have now pushed it as c48ec81 in PR #58, with a little rewrite of the commit message (to add an empty line after the 1st line and fix punctuation).

olexandr-konovalov · 2020-04-10T09:05:52Z

This PR was an intermediate step, I have now replaced it by #58 and will provide instructions for further work at https://github.com/gap-packages/wedderga/wiki (basically, I called the branch div-alg-bug and did not like the name, since this project goes far beyond a bugfix).

drallenherman · 2020-04-10T13:20:05Z

I committed and pushed my fix bug in RCA causing problems with GSOCA.
It should not cause a problem now.
Hope to have it done properly this time. At least all looked correct.

Yes it is working smoothly now on SG(160,137).
I am still wondering about the two F2's in line 2487 of div-alg.gi.
Alex and I were thinking one of them should be an F1?

olexandr-konovalov force-pushed the div-alg-bug branch 6 times, most recently from 8d01ad1 to d73204b Compare April 9, 2020 14:41

olexandr-konovalov force-pushed the div-alg-bug branch 2 times, most recently from 287ee6f to 2e35874 Compare April 9, 2020 14:54

olexandr-konovalov force-pushed the div-alg-bug branch 6 times, most recently from 8dff8ee to f6bb154 Compare April 9, 2020 17:36

olexandr-konovalov mentioned this pull request Apr 9, 2020

Allen's fixes in LocalIndexAtOddP and LocalIndexAtTwo #52

Closed

olexandr-konovalov mentioned this pull request Apr 9, 2020

Incorporated div-alg changes by Allen and Ángel #56

Closed

olexandr-konovalov changed the title ~~Refactoring of PR 56~~ Overhaul of div-alg code (refactoring of PR 56) Apr 9, 2020

olexandr-konovalov mentioned this pull request Apr 9, 2020

Test bugfix from #52 #55

Merged

Alexander Konovalov and others added 6 commits April 9, 2020 19:32

Initial import of div-alg changes by Allen and Ángel

d920c0a

Update version number in PackageInfo.g

dda45f1

We will temporarily use this version to distinguish the package loaded from this branch

Remove trailing whitespaces

70ed4d9

This was done accidentally because of default Atom settings.

Document Global Splitting and Character Descent Functions

731fcfe

Also extend LocalIndicesOfCyclotomicAlgebra mansection and save the file without trailing whitespaces.

Fix GaloisRepsOfCharacters; Improve GlobalCharacterDescent; Document …

4636f54

…new commands

olexandr-konovalov force-pushed the div-alg-bug branch from f6bb154 to 4636f54 Compare April 9, 2020 18:32

olexandr-konovalov commented Apr 9, 2020

View reviewed changes

olexandr-konovalov closed this Apr 10, 2020

drallenherman mentioned this pull request Apr 10, 2020

Overhaul of div-alg code #58

Merged

4 tasks

olexandr-konovalov deleted the div-alg-bug branch April 15, 2020 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul of div-alg code (refactoring of PR 56) #57

Overhaul of div-alg code (refactoring of PR 56) #57

olexandr-konovalov commented Apr 9, 2020 •

edited

Loading

codecov bot commented Apr 9, 2020 •

edited

Loading

olexandr-konovalov commented Apr 9, 2020

drallenherman commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020 •

edited

Loading

olexandr-konovalov commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020 •

edited

Loading

angeldelriomateos commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

angeldelriomateos commented Apr 9, 2020

angeldelriomateos commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

drallenherman commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

drallenherman commented Apr 9, 2020 via email

olexandr-konovalov Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

angeldelriomateos commented Apr 10, 2020

olexandr-konovalov commented Apr 10, 2020

olexandr-konovalov commented Apr 10, 2020

drallenherman commented Apr 10, 2020

Overhaul of div-alg code (refactoring of PR 56) #57

Overhaul of div-alg code (refactoring of PR 56) #57

Conversation

olexandr-konovalov commented Apr 9, 2020 • edited Loading

codecov bot commented Apr 9, 2020 • edited Loading

Codecov Report

olexandr-konovalov commented Apr 9, 2020

drallenherman commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020 • edited Loading

olexandr-konovalov commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020 • edited Loading

angeldelriomateos commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

angeldelriomateos commented Apr 9, 2020

angeldelriomateos commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

drallenherman commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

drallenherman commented Apr 9, 2020 via email

olexandr-konovalov Apr 9, 2020

Choose a reason for hiding this comment

olexandr-konovalov commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

olexandr-konovalov commented Apr 9, 2020

angeldelriomateos commented Apr 10, 2020

olexandr-konovalov commented Apr 10, 2020

olexandr-konovalov commented Apr 10, 2020

drallenherman commented Apr 10, 2020

olexandr-konovalov commented Apr 9, 2020 •

edited

Loading

codecov bot commented Apr 9, 2020 •

edited

Loading

olexandr-konovalov commented Apr 9, 2020 •

edited

Loading

olexandr-konovalov commented Apr 9, 2020 •

edited

Loading