Fix One issue
#4768
Fix One issue
#4768
Conversation
james-d-mitchell
force-pushed
the
fix-one-issue
branch
from
1783e1f
to
fb4d5ab
Compare
james-d-mitchell
force-pushed
the
fix-one-issue
branch
from
fb4d5ab
to
a1d350b
Compare
|
Are we sure that removing the method in question from |
|
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. |
| <Example><![CDATA[ | ||
| gap> f := PartialPerm( [ 1, 2, 3, 6, 8, 10 ], [ 2, 6, 7, 9, 1, 5 ] );; | ||
| gap> S := Semigroup(f, One(f)); | ||
| <commutative partial perm monoid of rank 9 with 1 generator> | ||
| <partial perm semigroup of rank 9 with 2 generators> | ||
| gap> IsMonoid(S); | ||
| true | ||
| false | ||
| gap> IsPartialPermMonoid(S); | ||
| true]]></Example> | ||
| false]]></Example> |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
I can no longer make sense of my comment here (at least the first half) so I've hidden it.
|
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 |
|
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); |
This PR resolves Issue #4763 and changes the behaviour of
SemigroupByGeneratorsto stop it from returning a monoid if theOneof the generators is one of the generators. This PR builds on #4766 and supersedes #4767.