-
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
Wreath product #262
Wreath product #262
Conversation
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 ok, there are a few things to change in the documentation, you consistently say semigroup
when I think you mean transformation semigroup
or maybe transformation monoid
. You also capitalise Wreath Product, which I wouldn't do.
doc/semitrans.xml
Outdated
<#GAPDoc Label="WreathProduct"> | ||
<ManSection> | ||
<Oper Name = "WreathProduct" Arg = "S, G"/> | ||
<Returns>The Wreath Product of <A>S</A> and <A>G</A>.</Returns> |
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.
No capital letters of Wreath or Product
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 comment applies in numerous places.
doc/semitrans.xml
Outdated
<Description> | ||
For a monoid <A>S</A> and a permutation group <A>G</A>, | ||
<C>WreathProduct(<A>S</A>, <A>G</A>)</C> outputs the semigroup Wreath | ||
product of S and G in terms of its embedding in the full transformation |
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.
Missing <A>
tags around S and G here.
doc/semitrans.xml
Outdated
For a monoid <A>S</A> and a permutation group <A>G</A>, | ||
<C>WreathProduct(<A>S</A>, <A>G</A>)</C> outputs the semigroup Wreath | ||
product of S and G in terms of its embedding in the full transformation | ||
monoid. For example, the Wreath product of T3, the full transformation |
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 don't use T3 or C2 (this is not a standard convention in the Semigroups package)
doc/semitrans.xml
Outdated
product of S and G in terms of its embedding in the full transformation | ||
monoid. For example, the Wreath product of T3, the full transformation | ||
monoid on three points, and C2, the group generated by the transposition | ||
<C>(1,3)</C>, would be output as its emedding in the full transformation |
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.
spelling mistake emedding
gap/semigroups/semitrans.gi
Outdated
function(S, G) | ||
local maps, gensS, next, reps, n, i, g, x, m; | ||
if not IsMonoidAsSemigroup(S) then | ||
ErrorNoReturn("S should be a monoid"); |
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 error should be in the format: Semigroups: WreathProduct: usage..
please see other error messages in the package for the correct format.
gap/semigroups/semitrans.gi
Outdated
m := LargestMovedPoint(G); | ||
maps := []; # the final generating set for the wreath product | ||
|
||
gensS := ShallowCopy(GeneratorsOfSemigroup(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.
I don't think you need to take a ShallowCopy
here.
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.
We do need ShallowCopy because we later modify the list gensS which would otherwise be immutable as GeneratorsOfSemigroup(S) returns an immutable object.
gap/semigroups/semitrans.gi
Outdated
od; | ||
od; | ||
fi; | ||
return Semigroup(maps); |
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.
Isn't the return value a monoid? If so, shouldn't we return Monoid(maps)
here?
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.
Even though it is a monoid, using Monoid(maps) adds the identity transformation, which may not be in the wreath product. That is why using Semigroup(maps) is the appropiate thing to do.
gap/semigroups/semitrans.gi
Outdated
@@ -735,6 +735,47 @@ function(S) | |||
return t; | |||
end); | |||
|
|||
InstallMethod(WreathProduct, "for a monoid and a permutation group", |
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 string here is wrong, it should describe the arguments in the next line. So in this case it should be for a transformation semigroup and a permutation group
.
08c6268
to
7ff5740
Compare
c22d5c6
to
e05def6
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.
I have some comments, mostly about the documentation.
gap/semigroups/semitrans.gi
Outdated
end); | ||
|
||
InstallMethod(WreathProduct, | ||
"for a transformation semigroup and a permutation group", |
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 string should be the other way around: "for a permutation group and a transformation semigroup"
doc/semitrans.xml
Outdated
<#GAPDoc Label="WreathProduct"> | ||
<ManSection> | ||
<Oper Name = "WreathProduct" Arg = "S, G"/> | ||
<Returns>The wreath product of <A>S</A> and <A>G</A>.</Returns> |
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 Returns
statement should just be the type of object returned: A transformation semigroup.
doc/semitrans.xml
Outdated
<Oper Name = "WreathProduct" Arg = "S, G"/> | ||
<Returns>The wreath product of <A>S</A> and <A>G</A>.</Returns> | ||
<Description> | ||
For a transformation monoid <A>S</A> and a permutation group <A>G</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.
Doesn't this work for any transformation semigroup?
doc/semitrans.xml
Outdated
|
||
<#GAPDoc Label="WreathProduct"> | ||
<ManSection> | ||
<Oper Name = "WreathProduct" Arg = "S, G"/> |
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.
We should also have the version where Arg = "S, G"
. You can just add the line right below this.
gap/semigroups/semitrans.gi
Outdated
rimage; | ||
if not IsMonoidAsSemigroup(S) then | ||
ErrorNoReturn("Semigroups: WreathProduct: usage,\n", | ||
"the first argument <S> should be a monoid,"); |
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.
<S>
is the second argument.
f57db4e
to
9b634e4
Compare
gap/semigroups/semitrans.gi
Outdated
end); | ||
|
||
InstallMethod(WreathProduct, | ||
"for two transformation 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.
Just one bad string here: you want "for a transformation monoid and a transformation 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.
Thank you for pointing that out. I have already fixed it!
I have one more suggestion: add a method for |
Michael, thank you for the suggestion to add the method for two permutation groups. GAP already has a method to do the wreath product of two groups, and if I were to add this method, there would appear the following warning: "method installed for WreathProduct matches more than one declaration". Should I add this method, regardless? |
@ffloresbrito no don't add the method for 2 perm groups, since it already exists in GAP there is no need.` |
Aha! I'm glad to hear a method already exists. In that case, please
ignore my suggestion.
…On 23 June 2017 at 12:36, Fernando Flores Brito ***@***.***> wrote:
Michael, thank you for the suggestion to add the method for two
permutation groups. GAP already has a method to do the wreath product of
two groups, and if I were to add this method, there would appear the
following warning: "method installed for WreathProduct matches more than
one declaration". Should I add this method, regardless?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#262 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFUy714in0FfQXt4e1WPW0nxztpz-E9sks5sG6MrgaJpZM4MLa-V>
.[image: Web Bug from
https://github.com/notifications/beacon/AFUy7_XhbBZ4lmu9unElh7spRVkkOtpdks5sG6MrgaJpZM4MLa-V.gif]
{"api_version":"1.0","publisher":{"api_key":"
05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":
{"external_key":"github/gap-packages/Semigroups","title":"
gap-packages/Semigroups","subtitle":"GitHub repository","main_image_url":"
https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-
11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://
cloud.githubusercontent.com/assets/143418/15842166/
7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in
GitHub","url":"https://github.com/gap-packages/Semigroups"}}
***@***.*** in
#262: Michael, thank you for the suggestion to add the method for two
permutation groups. GAP already has a method to do the wreath product of
two groups, and if I were to add this method, there would appear the
following warning: \"method installed for WreathProduct matches more than
one declaration\". Should I add this method, regardless?
"}],"action":{"name":"View Pull Request","url":"https://
github.com/gap-packages/Semigroups/pull/262#issuecomment-310643488"}}}
|
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.
Looks good apart from the suggested changes.
doc/semitrans.xml
Outdated
<Description> | ||
If <A>M</A> is a transformation monoid or a permutation group, and | ||
<A>S</A> is a transformation semigroup or a permutation group, the | ||
function <C>WreathProduct(<A>M</A>, <A>S</A>)</C> outputs the wreath |
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 function <C>WreathProduct(<A>M</A>, <A>S</A>)</C> outputs
-> the operation <C>WreathProduct</C> returns
for consistency with the rest of the manual.
doc/semitrans.xml
Outdated
<A>S</A> is a transformation semigroup or a permutation group, the | ||
function <C>WreathProduct(<A>M</A>, <A>S</A>)</C> outputs the wreath | ||
product of <A>M</A> and <A>S</A>, in terms of its embedding in the full | ||
transformation monoid. |
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.
You could also be much more specific about what this embedding actually is.
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.
But there is still no information about what the embedding actually is.
doc/z-chap13.xml
Outdated
<Section> | ||
|
||
<Heading> | ||
The wreath product of a transformation monoid and a permutation group |
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 think this should be in the same section as the documentation for the direct product, if that exists.
gap/semigroups/semitrans.gi
Outdated
[IsTransformationMonoid, IsPermGroup], | ||
function(M, G) | ||
local S; | ||
S := AsMonoid(IsTransformationMonoid, G); |
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 could just be return WreathProduct(M, AsMonoid(IsTransformationMonoid, G))
gap/semigroups/semitrans.gi
Outdated
function(G, S) | ||
local M; | ||
M := AsMonoid(IsTransformationMonoid, G); | ||
return WreathProduct(M, 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.
Same as previous comment.
gap/semigroups/semitrans.gi
Outdated
rimage; | ||
if not IsMonoidAsSemigroup(S) then | ||
ErrorNoReturn("Semigroups: WreathProduct: usage,\n", | ||
"the second argument <S> should be a monoid,"); |
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.
"should be a monoid (as semigroup)"
gap/semigroups/semitrans.gi
Outdated
gensM := List(gensM, x -> OnTuples([1 .. m], x)); | ||
gensS := GeneratorsOfSemigroup(S); | ||
|
||
orbs := List(ComponentsOfTransformationSemigroup(S), x -> Minimum(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.
Can't lines 779 to 787 be replaced with rimage := DigraphSources(DigraphOfActionOnPoints(S));
?
#T# Test wreath product of perm. group and transf. semgp. | ||
gap> W := WreathProduct(Group((1, 2)), FullTransformationMonoid(3));; | ||
gap> Size(W); | ||
216 |
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 maybe test some more things than just the size too?
883b2de
to
f173aa3
Compare
47358bc
to
17638ac
Compare
gap/semigroups/semitrans.gi
Outdated
rimage; | ||
if not IsMonoidAsSemigroup(S) then | ||
ErrorNoReturn("Semigroups: WreathProduct: usage,\n", | ||
"the second argument <S> should be a monoid (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.
Lines 778 and 779 should be aligned correctly.
gap/semigroups/semitrans.gi
Outdated
rimage; | ||
if not IsMonoidAsSemigroup(S) then | ||
ErrorNoReturn("Semigroups: WreathProduct: usage,\n", | ||
"the second argument <S> should be a monoid (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.
Align lines 778 and 779 properly.
gap/semigroups/semitrans.gi
Outdated
m := DegreeOfTransformationCollection(M); | ||
|
||
gensM := ShallowCopy(GeneratorsOfMonoid(M)); | ||
gensM := List(gensM, x -> OnTuples([1 .. m], 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.
I'd replace these 2 lines with
gensM := List(GeneratorsOfMonoid(M), x -> ImageListOfTransformation(x, m));
gap/semigroups/semitrans.gi
Outdated
gensM := List(gensM, x -> OnTuples([1 .. m], x)); | ||
gensS := GeneratorsOfSemigroup(S); | ||
|
||
orbs := List(ComponentsOfTransformationSemigroup(S), x -> Minimum(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.
This can be simplified! Replace x -> Minimum(x)
with just Minimum
gap/semigroups/semitrans.gi
Outdated
|
||
gen1 := gensS[1]; | ||
for i in orbs do | ||
newmap := OnTuples([1 .. m * n], maps[1]); |
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.
As before, try changing this to newmap := ImageListOfTransformation(maps[1], m * n);
This pull request adds a method for the operation
WreathProduct
that computes the wreath product of a monoid and a permutation group as its embedding in the full transformation monoid. Documentation and tests are included.