-
Notifications
You must be signed in to change notification settings - Fork 36
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
Improved direct products, with new supporting methods #347
Conversation
44119af
to
f066b03
Compare
f066b03
to
ccdba4c
Compare
82bd029
to
fc752b0
Compare
This is almost ready for merging, I just need to complete my proof that the algorithm is correct. |
844e300
to
7743fe4
Compare
dac7e51
to
10cd103
Compare
10cd103
to
d1ca8fd
Compare
It took far longer to prove than I thought it would, but I'm now confident that this PR is mathematically sound, and therefore ready to merge. I'd be grateful for reviews! |
c4e7d49
to
6bb08be
Compare
5d1d0fa
to
94709a3
Compare
@mtorpey and I are both using up-to-date |
Ok, so this also happens when the Semigroups package is not loaded at all |
So, it seems to be an issue in GAP itself, if the first group of tests is commented out. |
If the contents of the test file
and I load GAP without Semigroups (or anything else loaded using
then I get, when
|
Just going to check if this still holds when the |
Can @wilfwilson, @flsmith and/or @mtorpey confirm? |
I'm going to file an issue |
@james-d-mitchell I get the exact same behaviour from that bug.tst |
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 please add some tests for Embedding
and Projection
? Sorry forget it, I see there are plenty of tests.
Thanks @flsmith |
@james-d-mitchell I got the same behaviour from |
@james-d-mitchell I can add some more visible tests though. |
Sorry @wilfwilson I didn't see the tests because github collapsed the entire file. I see it is well tested! I'll have a look through, then hopefully we'll get the CI to work, then merge. |
Just saw your updated message @james-d-mitchell - I'll only add the test that I'm already working on now. |
6010627
to
7e86d99
Compare
7e86d99
to
123eaf0
Compare
@james-d-mitchell I'm pleased to report that Travis now passes, thanks to @ChrisJefferson's fix. |
Awesome, I'll look this over again today. |
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 minor change to the doc.
direct product. <P/> | ||
|
||
If these finite semigroups are either all partial perm semigroups, or all | ||
bipartition semigroups, or all PBR semigroups, then <C>DirectProduct</C> |
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.
Also if they are transformation semigroups, no? Also remove "either" since there are more than two possibilities, and also remove the first "or"
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 true that if the args are all transformation semigroups, then the result is a transformation semigroup. But that falls under the next sentence: Otherwise, <C>DirectProduct</C> returns 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.
Right you are, I'll merge this just now
This PR includes several related changes. Most importantly:
DirectProductOp
, for a list of either transformation semigroups, or bipartition semigroups, or partial perm semigroups.Previously,
DirectProductOp
methods existed for:This PR extends this functionality to support all partial perm semigroups, and all bipartition semigroups. It does this with a unified function. Notably, this function replaces the previous method for arbitrary finite transformation semigroups, which used a brute force search to find a generating set for the direct product.
My new method sometimes returns a semigroup with a larger generating set than the previous method, but it finds the answer far faster. To see an example of improved performance, try calculating the direct product of the singular transformation semigroup of degree 4 with itself. This takes 40ms with the new code, and gives a semigroup with 96 generators. The old code takes 4800ms, although it does only use 53 generators. Try a higher degree and the time difference is far starker. The generators returned by the new method can be reduced by
SmallGeneratingSet
, or similar, if desired.I have ideas for reducing the number of generators returned my method, but they're still only ideas, and in any case, they would require my (future) pull request concerning the partial order of L- and R-classes of a semigroup, and in particular, maximal L- and R-classes.
The new direct product code is reliant on the new methods called:
NonTrivialFactorization
.Also introduced are the related new functions:
IsDecomposableSemigroup
(i.e. isS^2 = S
?)IndecomposableElements
(i.e. those elements inS \ S^2
)The indecomposable elements of
S
are precisely those elements inGeneratorsOfSemigroup(S)
without a non-trivial factorization in the semigroup.