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

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

Closed olexandr-konovalov closed 4 years ago

olexandr-konovalov commented 4 years ago

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

codecov[bot] commented 4 years ago

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%) :arrow_down:
lib/div-alg.gd 100.00% <100.00%> (ø)
lib/crossed.gi 73.77% <0.00%> (-0.15%) :arrow_down:
olexandr-konovalov commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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:

Are you happy with this plan?

olexandr-konovalov commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

Can I commit my fix of a bug on ReducingCyclotomicAlgebra?

olexandr-konovalov commented 4 years ago

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

angeldelriomateos commented 4 years ago

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

angeldelriomateos commented 4 years ago

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

olexandr-konovalov commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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.

olexandr-konovalov commented 4 years ago

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

drallenherman commented 4 years ago

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 notifications@github.com 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: https://github.com/gap-packages/wedderga/pull/57#issuecomment-611676269

olexandr-konovalov commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

angeldelriomateos commented 4 years ago

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 commented 4 years ago

@angeldelriomateos thanks, it seems your commit went smoothly. I have now pushed it as c48ec81611b93879e1fe1a988fff004894bd0a6c 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 commented 4 years ago

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 commented 4 years ago

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?