gap
gap copied to clipboard
Fix `One` issue
This PR resolves Issue #4763 and changes the behaviour of SemigroupByGenerators to stop it from returning a monoid if the One of the generators is one of the generators. This PR builds on #4766 and supersedes #4767.
Are we sure that removing the method in question from lib/arith.gi will not break some users' code?
(I agree that this method is likely to do something that is not intended, but perhaps it is too brave to just remove it.)
Hi @james-d-mitchell thanks for the PR, wasn't expecting you to spend time on this :-).
But I agree with @ThomasBreuer, I think we need to be a bit careful. E.g. the Semigroups package definitely relies on this method (I just checked), and others might as well. Before removing this, I'd rather first have the ability again to run tests of all distributed packages (to be tackled during GAP Days next week); then we can update packages; and then maybe we can think about it for GAP 4.13 -- although that doesn't solve the issue of people possibly having unpublished code relying on this...
Thanks for your comments, @fingolfin and @ThomasBreuer. I agree this is probably too big a change to make all at once, perhaps the right thing to do is to merge #4767, and to leave this PR open to be merged at some point in the future?
@james-d-mitchell Thanks. Yes, eventually this PR should be merged, but for the moment merging just #4767 is safer.
I've merged Thomas' PR #4767 for now.
To make progress on this PR here, I think first we need to adjust any packages currently relying on these One methods -- hopefully that's "just" semigroups. Then once there is a Semigroups release which doesn't need it anymore in the package distro, we could apply this PR.
Should this PR also remove these methods?
InstallMethod(OneImmutable, "for a partial perm coll",
[IsPartialPermCollection],
function(x)
return JoinOfIdempotentPartialPermsNC(List(x, OneImmutable));
end);
InstallMethod(OneMutable, "for a partial perm coll",
[IsPartialPermCollection], OneImmutable);