-
Notifications
You must be signed in to change notification settings - Fork 35
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
Implement dual semigroups #470
Conversation
3d85769
to
f62b280
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, apart from a few niggles. There are only two serious points:
- Please get rid of
DualSemigroupElement
and theNC
version, and simply replace them with uses ofAntiIsomorphicDualSemigroup
- Perhaps rename
IsDualSemigroup
toIsDualSemigroupRep
, so that it is clear that it is not a property of a semigroup but just a representation of one. This would involve makingIsDualSemigroup
a representation, in which you could store the type and family of the elements as internal details, rather than explicitly as attributes.
Also please add a comment in the .gi
file that says that basically we implement the bare minimum to make a DualSemigroup
belong to IsEnumerableSemigroupRep
and then fall back on the methods for enumerable semigroups, and that this is done so that we do not have to maintain a method for every single feature of the Semigroups package just for dual semigroups (although this might be faster for some methods).
Also the two commits should be squashed.
doc/dual.xml
Outdated
<Description> | ||
This method returns a representation of the dual semigroup of | ||
<A>sgrp</A>. The dual semigroup of a semigroup <A>S</A> is the | ||
antisomorphic semigroup with the same underlying set as <A>S</A>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anti-isomorphic
doc/dual.xml
Outdated
|
||
<#GAPDoc Label="DualSemigroup"> | ||
<ManSection> | ||
<Attr Name = "DualSemigroup" Arg = "sgrp"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arg = "S"
doc/dual.xml
Outdated
<Attr Name = "DualSemigroup" Arg = "sgrp"/> | ||
<Returns>The dual semigroup of the given semigroup.</Returns> | ||
<Description> | ||
This method returns a representation of the dual semigroup of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Returns a semigroup isomorphic to the dual semigroup of S." It isn't a method, it's an attribute, and it returns a semigroup isomorphic to the dual semigroup rather than a representation of the dual semigroup.
gap/attributes/dual.gd
Outdated
DeclareGlobalFunction("DualSemigroupElementNC"); | ||
DeclareGlobalFunction("UnderlyingElementOfDualSemigroupElement"); | ||
|
||
DeclareAttribute("AntiIsomorphismDualSemigroup", IsSemigroup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AntiIsomorphismDualSemigroup
does not seem to be documented. I'd scrap both DualSemigroupElement
and DualSemigroupElementNC
and simply use AntiIsomorphismDualSemigroup
instead.
gap/attributes/dual.gd
Outdated
DeclareAttribute("DualSemigroup", IsSemigroup); | ||
DeclareSynonym("IsDualSemigroup", IsSemigroup and | ||
IsDualSemigroupElementCollection); | ||
DeclareAttribute("TypeDualSemigroupElements", IsDualSemigroup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type and the family should probably be internal parts of the representation of IsDualSemigroup
rather than visible attributes as here.
gap/attributes/dual.gi
Outdated
############################################################################# | ||
## | ||
#W dual.gi | ||
#Y Copyright (C) 2017 James D. Mitchell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's 2018 now 👍
gap/attributes/dual.gi
Outdated
return Semigroup(List(GeneratorsOfSemigroup(S), | ||
x -> UnderlyingElementOfDualSemigroupElement(x))); | ||
fi; | ||
# FS: need to work out what to do if not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better have an error rather than fail in some weird way here.
gap/attributes/dual.gi
Outdated
|
||
if IsTransformationSemigroup(S) then | ||
SetAntiIsomorphismTransformationSemigroup(dual, | ||
AntiIsomorphismDualSemigroup(dual)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The A
should be inline with the (
in the previous line.
gap/attributes/dual.gi
Outdated
|
||
if HasGeneratorsOfMonoid(S) then | ||
SetGeneratorsOfMonoid(dual, List(GeneratorsOfMonoid(S), | ||
x -> DualSemigroupElementNC(dual, x))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation
gap/attributes/dual.gi
Outdated
S := DualSemigroupOfFamily(FamilyObj(s)); | ||
return DualSemigroupElementNC(S, | ||
OneMutable(DualSemigroupElement( | ||
DualSemigroup(S), s))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation
doc/z-chap06.xml
Outdated
@@ -360,6 +360,10 @@ true]]></Log> | |||
<#Include Label = "InverseSubsemigroupByProperty"> | |||
<#Include Label = "DirectProduct"> | |||
<#Include Label = "WreathProduct"> | |||
<#Include Label = "DualSemigroup"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be tempted to put the dual semigroups stuff into a section by itself.
1542df1
to
524257e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good, some minor points to fix before merging, and one other thing: can you please add some tests along the lines of BruteForceIso/InverseCheck
(grep for these) to ensure that the returned mappings are actually mutually inverse anti-isomorphisms?
doc/dual.xml
Outdated
<ManSection> | ||
<Attr Name= "AntiIsomorphismDualSemigroup" Arg = "S"/> | ||
<Returns> | ||
A mapping from <A>S</A> to corresponding elements in the dual semigroup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"an anti-isomorphism from S to the corresponding dual semigroup"
doc/dual.xml
Outdated
<Description> | ||
The dual semigroup of <A>S</A> mathematically has the same underlying | ||
set as <A>S</A>, but is represented with a different set of elements in | ||
Semigroups. This function returns a mapping which is an anti-isomorphism from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Semigroups" -> "&Semigroups;"
doc/dual.xml
Outdated
<#GAPDoc Label="IsDualSemigroupElement"> | ||
<ManSection> | ||
<Filt Name = "IsDualSemigroupElement" Type = "Category" Arg="elt"/> | ||
<Returns>Returns true if <A>elt</A> has the representation of a dual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"true" should have K-tags around it.
doc/dual.xml
Outdated
<Description> | ||
Elements of a dual semigroup obtained using | ||
<Ref Attr = "AntiIsomorphismDualSemigroup"/> normally lie in this | ||
category. The exception is dual elements to elements that already |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The exception is elements dual to those already in the category"?
doc/dual.xml
Outdated
<#GAPDoc Label="IsDualSemigroupRep"> | ||
<ManSection> | ||
<Filt Name = "IsDualSemigroupRep" Type = "Category" Arg="sgrp"/> | ||
<Returns>Returns true if <A>sgrp</A> is represented as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K-tags
doc/z-chap06.xml
Outdated
|
||
<Section> | ||
<Heading> Dual semigroups </Heading> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a sentence here defining dual semigroups?
gap/attributes/dual.gd
Outdated
############################################################################# | ||
## | ||
## dual.gd | ||
## Copyright (C) 2017 James D. Mitchell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2018!
gap/attributes/dual.gi
Outdated
## | ||
############################################################################# | ||
## | ||
## This file contains an implementation of dual semigroups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reflow this comment
gap/attributes/dual.gi
Outdated
|
||
if IsTransformationSemigroup(S) then | ||
SetAntiIsomorphismTransformationSemigroup(dual, | ||
AntiIsomorphismDualSemigroup(dual)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation? If the resulting line is too long, then assign map := AntiIsomorphismDualSemigroup(dual);
in the previous line
gap/attributes/dual.gi
Outdated
local S; | ||
S := DualSemigroupOfFamily(FamilyObj(s)); | ||
return SEMIGROUPS.DualSemigroupElementNC(S, | ||
OneMutable( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation?
@flsmith it looks like you need to rebase on master (pushed just moments ago). |
6e1da26
to
57be6f7
Compare
Hohum, the tests in master are failing at present, I'm fixing it, will comment when it's back to normal. |
The tests in |
9105017
to
7669de1
Compare
This PR implements dual semigroups. The dual semigroup D of a semigroup S is the semigroup with the same underlying set of elements, but reversed multiplication; i.e. the product s * t in S is the same as the product t * s in the dual semigroup.
The implementation chosen is to simply wrap elements in a new category IsDualSemigroupElement. A few quirks arise from this - the dual of a semigroup satisfying IsDualSemigroup will not be a dual semigroup, for example.
The functionality provided should cover all basic uses of dual semigroups (in particular, Froidure Pin works). If further requirements arise it is generally quite easy to extend the functionality.