-
Notifications
You must be signed in to change notification settings - Fork 176
Semigroups #317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Semigroups #317
Conversation
|
Thanks - this contains 22 commits and cumulative diff also shows many changes which are only removing traling whitespace characters. Could you perhaps point out to individual commits among those 22 to help reviewing this? Will this break any packages that you maintain? Should this be merged simultaneously with their coming updates? Is this important to do prior to GAP 4.8 beta as it may break tests in other packages, to avoid the necessity of the 2nd beta? |
|
If you go to https://github.com/gap-system/gap/pull/317/files?w=1 , you can see this without the whitespace changes. I see at the bottom a couple of '#FIXME' comments. Should they get fixed? :) |
|
I updated the comments in the pull request to say what we did. Please let me know if I should do anything else! |
|
We have the necessary changes to Semigroups and Smallsemi in the development versions and will make a release probably after the release of the 4.8 beta. |
|
@james-d-mitchell thanks - but are current Semigroups and Smallsemi compatible with these changes? There are packages that may rely on them. |
|
No the current versions of Semigroups and Smallsemi are broken by these changes. It is easy to resolve these problems though, so I could release new versions of both packages essentially immediately. Would this be the best thing to do? |
|
@james-d-mitchell I suggest to get this PR reviewed first, and if approved, merge it and then immediately release Semigroups and Smallsemi. Just in case there may be some adjustments needed after review. |
|
This PR contains a ton of pointless merge commits in this branch, which complicate reviewing. It would hence be really nice if you could at the very least rebase this on the latest master, and drop the pointless merges. This is super easy to do (I just tried it with this PR on my local machine): This leaves me with 10 commits. You may also want to pass Once you did that, you can use |
|
Some immediate remarks:
|
|
And lest I sound too negative: Thank you for your work on this, it seems overall like a good thing to me, and I appreciate it! |
|
Thanks for your comments @fingolfin, I'll get rid of the merges in the PR as you suggest, and add I completely agree that it would make more sense to have Smallsemi depend on Semigroups, and to have all of the declarations in Semigroups, and not in the library. Unfortunately, my co-author for Smallsemi didn't want to make Semigroups a dependency, and so my hands were tied. For a variety of reasons, I'd really like to make Smallsemi depend on Semigroups, I'll try to convince my co-author again. Just to mention that there are quite some things in the library (for Green's *-relations, and property testing) that are only declared (no methods installed in the library) so that they can be used in both Semigroups and Smallsemi without making Smallsemi dependent on Semigroups, not only |
|
@james-d-mitchell OK, I see. Regarding other things in the library that are only for use in Semigroups and Smallsemi: Of course I'd then also prefer if those were moved to Semigroups :). But this not urgent. And we have more of these kind, e.g. if you grep for |
8466db4 to
16370ff
Compare
|
I think I've now made the changes requested by @fingolfin: rebased, added a synonym to obsolete.gd for I think this need some further changes to |
|
@james-d-mitchell You may keep it open. Maybe add (WIP, do not merge) in the title of the pull request. The PR will be updated automatically as soon as you will update the branch from which is was made. |
|
As @alex-konovalov says :). It also seems some of those merge commits are back, so perhaps rebase it once more onto |
1ae31d1 to
bdea4ce
Compare
|
@fingolfin @alex-konovalov @ChrisJefferson I think this is ready to go, I've rebased the commits so that (hopefully) the changes are clear, I've added tests where appropriate, and I've update the documentation. I have versions of Semigroups and Smallsemi which are compatible with the changes in this PR. Please let me know if there's anything further I should do. Rebasing as suggested by @fingolfin doesn't seem to remove the merges from master into the semigroups branch, maybe I shouldn't have done that, not sure if this should be cleaned up or not? |
|
@james-d-mitchell That's odd, I just once again did a "rebase", and it reliably gets rid of the merges. That said, your branch is not based on latest master -- perhaps you did a |
|
Another nitpick: Your commit messages are non-standard. Please take a look at http://chris.beams.io/posts/git-commit/#seven-rules -- the main issue I have is that the first lines are far too long, and that the rest of the message also has lines that are far too long. You can fix this easily (well, at least from my perspective :) using |
bdea4ce to
5a9b3f0
Compare
70e61c5 to
cc06a18
Compare
|
I've now made the changes to the |
|
@james-d-mitchell I commented on the individual commits. Since you rebased and force pushed, those commits are now of course gone resp. have new ideas, so GitHub does not show my comments for them anymore. You can still view them if you know the old commit ids, though -- or open one of those notification emails you may have received for each of my comments, the links therein should still allow you to view those comments. |
|
Thanks @fingolfin just want to check that this was the correct thing to do. I believe I've addressed all of your comments, please let me know if I should do something further. |
|
@james-d-mitchell Thanks for your patience and your work. I think this patch series looks very good now. Well-done! I don't have any objections to it. Assuming that other people working on semigroups like @markuspf are fine with it (which I assume), I see no reason not to merge it. Though perhaps another Anyway, short version: Good to go. I'll run the full test suite on this now, and assumign I find no error, will merge it (unless somebody else beats me to it :) |
|
@fingolfin Milestones are useful, but they don't address the need completely - it's hard to judge the scope of each issue and PR listed there just from their titles sometimes not descriptive enough. That's why a high-level overview of changes is needed. |
|
This all looks good to me, and I will merge it now. Thanks for your patience @james-d-mitchell. |
|
@james-d-mitchell please now publish Semigroups (2.7) and Smallsemi (0.6.9). |
|
Just to inform you that this PR requires updating of a dozen of manual examples. I will do this as soon as other tests will stabilise. |
|
My request to change There was no proper explanantion as to why this trivial change was refused, hence I now made it myself in cc799f0 Let me explain in more detail why I think this is important: First off, generally speaking, It is always a bad idea to make a change that requires one to update to things simultaneously to make it work. For example, it complicates Of course, sometimes, there is no choice, and one is forced to do this unfortunate kind of "lock-step" change. But in this case, there is a trivial change that avoids this issue, requiring a single line change. Nobody identified any reason why this change should not be made, hence I now made it myself. |
|
On Tue, Nov 24, 2015 at 08:04:58AM -0800, Max Horn wrote:
This was me being stupid and not remembering why this PR was not merged yet. |
|
@fingolfin @markuspf I also completely agree with the rationale behind the change from @fingolfin thanks for all of your help with this pull request, it is very much appreciated. @alex-konovalov Sorry for the broken manual examples, I thought I had caught all of these. Please let me know if I should do anything further! |
|
@james-d-mitchell of course, now there are 37 diffs in testinstall.g but only when all packages are loaded. I think I only have to wait until your two new packages will be picked up, and they will hopefully gone... |
|
@james-d-mitchell the dev/Updates/is-group-as-semigroup file does not pass and with all packages (including Semigroups and Smallsemi picked up this morning) |
|
@james-d-mitchell please ignore the last diff with all packages - that is still with the stable version of semigroups (https://github.com/gap-system/gap-distribution/blob/master/DistributionUpdate/PackageUpdate/Makefile). I will recheck this will the latest now. But the 1st two diffs are clearly genuine. |
Also discovered some Example and Log elements not using CDATA and fixed them.
Also discovered some Example and Log elements not using CDATA and fixed them.
Also discovered some Example and Log elements not using CDATA and fixed them.
|
@james-d-mitchell just to give you the latest account:
|
Updated manual examples changed after PR #317
|
@james-d-mitchell just to remind that in |
|
@james-d-mitchell Never mind, I've figured out that |
This pull request contains a variety of minor changes to the semigroups functionality in GAP (these changes were made by Wilf Wilson, Michael Torpey and me, rebased for clarity). The changes are (in order of the commits):
Making it so that a submagma defined by no generators has
IsEmptynotIsTrivial. According to the manualIsTrivialimplies that the magma being created has exactly one element, but if it has 0 generators, then it has size 0. This was highlighted by the changes toViewStringfor a semigroup in (9) below.We removed the setting of
GeneratingPairsOfMagmaCongruencefor left or right (but not two-sided) congruences. This is nonsense and should never have been done, it's like settingGeneratorsOfGroupfor a semigroup which is not a group.Introducing the c functions
EqPermTrans22andEqPermTrans44for checking equality of two permutations, or two transformations, as discussed in PR Improve performance of EqPerm22 and EqPerm44 #280We declared the attribute
NilpotencyDegree, which was previously declared in Smallsemi. This is done to avoid warnings when loading Smallsemi/Semigroups.Fixed some inconsistencies, namely in the meaning of the categories/properties
IsSomethingAsSemigroup.IsMonoidAsSemigroupused to mean a semigroup that happens to be a group mathematically, but did not belong to the categoryIsMonoid(for whatever reason). On the other hand,IsGroupAsSemigroupwastruefor semigroups in the categoryIsGroup, and this was inconsistent withIsMonoidAsSemigroup.Also we renamed
IsSemilatticeAsSemigrouptoIsSemilatticeagain for reasons of consistency with the otherIsSomethingAsSemigroupproperties. The issue in this case is that there is no categoryIsSemilattice. This will cause problems in loading the current release versions of the Semigroups and Smallsemi package. I doubt these are used in other packages, but I might be wrong!We removed duplicate code and otherwise cleaned up
IsomorphismReesMatrixSemigroup, and introducedIsomorphismReesZeroMatrixSemigroupto avoid the previous situation where the range of the value returned byIsomorphismReesMatrixSemigroupwas not a Rees matrix semigroup.Updated the function
ReesZeroMatrixSemigroupso that Rees 0-matrix semigroups know that they are finite if the underlying semigroup is finite.Added proper print methods for Rees 0-matrix semigroup elements, that print valid GAP input, previously they did not.
Introducing a function for producing
ViewStringfor semigroups and monoids (in particular, transformation and partial perm semigroups), and another function for groups of the same.This changes the
ViewObjof transformation and partial perm slightly to remove "pts" and replace it with degree/rank for better readability, the previous methods are consolidated into a single function, the old code is removed, the tests and documentation are updated, and a new method forRankOfPartialPermSemigroupfor a partial perm semigroup with generators of a group (this is used by the new ViewString method).The immediate method for
GeneratorsOfSemigroupfor inverse semigroups is improved to not include obviously duplicate generators.Semigroup/Monoid/InverseSemigroup/InverseMonoidare fixed to prevent inappropriate removal of theOne(if present, and the only generator specified), and tests for this.Previously, if the
Oneof a collectioncollof generators was present incoll, and we tried to make a semigroup/monoid/inverse semigroup or inverse monoid generated bycoll, then theOnewas removed. In the case that theOnewas the only generator, or that theOneofcollwas not the same as theOneofcollwithOneremoved (possible for partial perms), this meant that the value ofGeneratorsOfMonoidcould not be used to recreate the object. For example,Semigroup(IdentityTransformation)formerly resulted in a monoid with 0 generators, butMonoid([])(rightly) returns an error (it is not possible to create monoids with 0 generators). This change fixes this, and adds some tests for this.