Skip to content
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

Don't use One on collections that are not domains #5155

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

This patch is a few months old, I think meant to workaround some problem with semigroups. I think this was resolved differently but I still think the change is generally a good idea...

@fingolfin fingolfin added topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes gapdays2022-summer Issues and PRs that arose at https://www.gapdays.de/gapdays2022-summer labels Oct 20, 2022
@fingolfin fingolfin changed the title Don't use One on collections that are not domains Don't use One on collections that are not domains Oct 21, 2022
@@ -289,7 +289,9 @@ true
# Test IsomorphismPartialPermMonoid for a partial perm semigroup
gap> S := Semigroup(PartialPerm([2], [2]), PartialPerm([1], [1]));;
gap> IsomorphismPartialPermMonoid(S);
Error, the argument must be a semigroup with a multiplicative neutral element
MappingByFunction( <partial perm monoid of rank 2 with 2 generators>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes no sense I’m afraid, the semigroup generated by the two partial perms given is not a monoid, and I think that with the changes in this pr it wouldn’t be the case that the return value of One was a multiplicative neutral element. I’m not sure what the motivation behind this is, and I’m pretty sure that making this change will badly break lots of the code for Semigroups

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it helps others reviewing, the identity returned by One is not the identity for the entire family of partial perms, but only for those partial perms with the same support. This is very different from the permutation case where all permutations have the same support (all positive integers). This means that One would need to be called on the list or the domain generated by the list, but this is the function that creates that domain, so seems hard to get right without calling it on a list.

gap> gens := [ PartialPerm([2], [2]), PartialPerm([1], [1]) ];
[ <identity partial perm on [ 2 ]>, <identity partial perm on [ 1 ]> ]
gap> One(gens);
<identity partial perm on [ 1, 2 ]>
gap> One(Representative(gens));
<identity partial perm on [ 2 ]>
gap> One(gens[1]);
<identity partial perm on [ 2 ]>

@ThomasBreuer
Copy link
Contributor

I understand this pull request as a step towards removing the One method in lib/arith.gi that takes an argument coll in the filter IsMultiplicativeElementWithOneCollection and returns One(Representative(coll)), provided that coll is not itself in the filter IsMultiplicativeElementWithOne.
I think it was a bad idea to have this method, and a comment in the code states that it causes logical problems.

The remark by @james-d-mitchell points to a second problem. Namely, lib/pperm.gi introduces another One method, which takes an argument coll in IsPartialPermCollection.
My interpretation is that the abovementioned One method for IsMultiplicativeElementWithOneCollection is not suitable for the situation of an IsPartialPermCollection (thanks to @jackschmidt for the explanation), but instead of avoiding the call of One in this situation, a different method of higher rank was installed that returns something reasonable.

I think the right solution would be to eventually get rid of both One methods, and for the moment to change the library code such that these methods are not used, in each case according to the One method in question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2022-summer Issues and PRs that arose at https://www.gapdays.de/gapdays2022-summer release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants