Collection of fixes for isomorphisms from semigroup to monoids #1112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Namely the methods for IsomorphismTransformationMonoid and IsomorphismPartialPermMonoid so that they always return monoids.
For semigroups/monoids of partial perms. Previously the inverse of the returned isomorphisms was not an inverse.
The previous methods for IsomorphismTransformationMonoid/Semigroups didn't work.
These methods were only trivially different, so I combined them into one method.
Remove redundant methods, add missing methods, update test output, and correct the documentation.
Also add comments to untested bits of the code.
This is to allow more methods to be installed, for example, so that things like: AsSemigroup(IsPartialPermSemigroup, S) can be used. Also updated the manual entry for AsMonoid so that it is more permissive, so that not only objects in the category IsMonoid can be the argument of AsMonoid.
for a transformation semigroup, so that it checks if the argument is mathematically a monoid not that it is in the category IsMonoid.
Current coverage is 57.06% (diff: 98.97%)@@ master #1112 diff @@
==========================================
Files 433 433
Lines 224666 224710 +44
Methods 3427 3427
Messages 0 0
Branches 0 0
==========================================
+ Hits 127832 128241 +409
+ Misses 96834 96469 -365
Partials 0 0
|
| dom := Set(set * InversesOfSemigroupElement(S, x)[1]); | ||
| return PartialPermNC(List(dom, y -> Position(set, y)), | ||
| List(List(dom, y -> y * x), | ||
| y -> Position(set, y))); | ||
| end; |
There was a problem hiding this comment.
This is annoying to read. As far as I can tell, you haven't actually changed anything in this part of the patch, just renamed variables and moved code around?
| function(s) | ||
| return MagmaIsomorphismByFunctionsNC(s, s, IdFunc, IdFunc); | ||
| function(S) | ||
| return MagmaIsomorphismByFunctionsNC(S, S, IdFunc, IdFunc); |
There was a problem hiding this comment.
Same here, why rename?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please make sure that this pull request:
Tick all what applies to this pull request
Write below the description of changes (for the release notes)
This PR contains a number of fixes relating to isomorphisms between semigroups and monoids. In addition, it changes
AsMonoidandAsSemigroupto operations, for future compatibility with the Semigroups package. There were numerous bugs in this code, and duplicated code. This PR cleans this up, fixes all these bugs, and adds numerous tests.