-
Notifications
You must be signed in to change notification settings - Fork 161
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
Improve documentation and argument checks for StabChainBaseStrongGenerators
#4602
Improve documentation and argument checks for StabChainBaseStrongGenerators
#4602
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.
LGTM. I've got a suggestion, but am not sure whether that's an improvement.
And I have no idea when When |
else | ||
one:= One(arg[2][1]); | ||
ErrorNoReturn("the identity element must be given as the third argument ", | ||
"when the second argument <sgs> is empty"); |
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... why? Why don't we simply always do one:= ()
?
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.
Good question, I have no idea. As I asked at the top of this thread:
(Can anyone think when you might want the one to not be ()?)
I can't think of a use case. Maybe @ChrisJefferson has an idea?
And @ssiccha – this function is documented to only apply to permutation groups, so it can't be because of 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.
Sorry, I missed that question at the top 😊
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 only package calling StabChainBaseStrongGenerators
is grape
and it also passes ()
as third argument.
But in TryPcgsPermGroup
, though, we pass U.identity
... That gave me a clue: we have
lib/memory.gi:108: S.identity := S.identity!.el;
So perhaps this is meant to allow supporting "permutations with memory" as produced by GeneratorsWithMemory
?
Looking at git blame
, the line in TryPcgsPermGroup
was added by @hulpke 21 years ago in this commit:
Author: Alexander Hulpke <hulpke@math.colostate.edu>
AuthorDate: 2000-01-25 18:50:09 +0000
Commit: Alexander Hulpke <hulpke@math.colostate.edu>
CommitDate: 2000-01-25 18:50:09 +0000
StabChainBaseStrongGenerators gets the appropriate `One' (this permits other
permutation representations). SM&AH.
Perhaps he recalls something?
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.
If we use other representations (e.g. elements with memory, or straight line program elements) the identity element will be given in this representation.
Please do not change this -- I am relying on it sometimes in private code.
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 intention to change it -- but clearly the documentation for this function then is wrong / incomplete
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.
Thanks for the explanation @hulpke, and don't worry, my purpose of the PR is to make the documentation match the behaviour of the code by changing the documentation, rather than to change the behaviour of the code.
However, I now feel a bit out of my depth, since I don't yet know anything about other representations for permutations in GAP.
I am also now worried that #4331 broke StabChainBaseStrongGenerators
for your private code @hulpke – could you please check? In particular, the assertion Assert(2, S.labels[1] = ());
is perhaps invalid, if a one
other than ()
is specified.
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.
That should (apart from anyhow being in an assertion) be harmless as objects with memory would have to compare as equal to ().
The reason for the different one
is if elements get created that will be used later on. By starting with the correct `one' allos to build a proper word expression on the way.
5ac7db1
to
14bdfc6
Compare
@ssiccha Yes a stabiliser chain can only have an empty set of strong generators if it is trivial. The 2-argument behaviour prior to this PR is:
So I feel that a specific error message is more helpful. A stabiliser chain needs to know its corresponding identity, so we can't guess it in that case. |
@@ -398,12 +398,20 @@ end); | |||
InstallGlobalFunction(StabChainBaseStrongGenerators,function(arg) | |||
local base,sgs,one,S, T, pnt, genlabels; | |||
|
|||
if not Length(arg) in [2, 3] | |||
or not ForAll([1, 2], i -> IsHomogeneousList(arg[i])) then |
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.
Is it valid to assume that valid bases and strong generating sets will be homogeneous lists?
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 it is but perhaps @hulpke and/or @ChrisJefferson may want to comment?
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 it's reasonable
This PR looks good to me. But I'd prefer if @hulpke could approve it, too (or point out any issues, of course!). If @ChrisJefferson also has time to look at it: even better :-) |
That's fine, there's certainly no rush to get this merged. From my point of view, this PR is doing three things:
|
I'm happy with this PR, as long as it doesn't break anything @hulpke is doing (I wouldn't expect so) |
I made a note to myself to document the 2-argument version of
StabChainBaseStrongGenerators
(which was already implemented and is probably what most people would want to use). I finally got around to it, and in the process I made a few other small improvements: added a manual example; added tests; added some argument checks.(Can anyone think when you might want the
one
to not be()
?)