gap icon indicating copy to clipboard operation
gap copied to clipboard

Fix `One` issue

Open james-d-mitchell opened this issue 3 years ago • 6 comments

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.

james-d-mitchell avatar Feb 14 '22 16:02 james-d-mitchell

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.)

ThomasBreuer avatar Feb 14 '22 22:02 ThomasBreuer

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...

fingolfin avatar Feb 14 '22 23:02 fingolfin

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 avatar Feb 15 '22 09:02 james-d-mitchell

@james-d-mitchell Thanks. Yes, eventually this PR should be merged, but for the moment merging just #4767 is safer.

ThomasBreuer avatar Feb 15 '22 09:02 ThomasBreuer

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.

fingolfin avatar Feb 19 '22 01:02 fingolfin

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);

fingolfin avatar Feb 19 '22 01:02 fingolfin