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

Fix RankOfPartialPermSemigroup for groups #3038

Merged
merged 1 commit into from
Nov 21, 2018

Conversation

wilfwilson
Copy link
Member

Previously, RankOfPartialPermSemigroup for a group of partial permutations would give an unexpected error if the GeneratorsOfGroup were empty (it delegated to RankOfPartialPermCollection(GeneratorsOfGroup(G))). A simpler things to do for groups, and for monoids actually, is to call RankOfPartialPerm(One(G)).

Resolves #3037.

@wilfwilson wilfwilson added kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them backport-to-4.10 labels Nov 21, 2018
@fingolfin fingolfin added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Nov 21, 2018
@fingolfin fingolfin merged commit 9ee988f into gap-system:master Nov 21, 2018
@wilfwilson wilfwilson deleted the fix-3037 branch November 21, 2018 14:30
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.1 milestone Nov 24, 2018
@olexandr-konovalov
Copy link
Member

@wilfwilson are you sure that this had to be backported? I did it, and now with Semigroups package (version 3.0.20) loaded I observe this in the stable-4.10 branch:

########> Diff in /circa/scratch/gap-jenkins/workspace/GAP-minor-release-test/\
GAPCOPTS/64build/GAPTARGET/install/label/kovacs/gap-4.10.1/tst/testinstall/sem\
ipperm.tst:90
# Input is:
RankOfPartialPermSemigroup(S);
# Expected output:
2
# But found:
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `RankOfPartialPermCollection' on 1 argum\
\
ents
########

@wilfwilson
Copy link
Member Author

wilfwilson commented Nov 25, 2018

@alex-konovalov Yes i wanted it to be backported to stable-4.10, although it didn’t have to be. However, I’m sorry, but I did not test whether the Semigroups package ran properly when this was backported, and unfortunately this causes a test failure, as you have found.

There are two reasons for this: in stable-4.10 the method ordering is still incorrect (this is largely/completely fixed in master now, thankfully). Because of this, for the newly added test in this PR, the incorrect method for DegreeOfPartialPermSemigroup is being applied. This is unfortunate, but the other, less desirable, applicable methods should still work. Secondly, the method that is being called instead causes an unexpected error, because of a newly discovered bug in the Semigroups package. So, this is caused by the method reordering problems in GAP and a bug in Semigroups, in combination.

I have fixed the bug in Semigroups here: semigroups/Semigroups#565 and so this problem will go away once that is merged and the next version of Semigroups is released.

If you are impatient for this test failure to be resolved, you could revert the backport to stable-4.10: I can live without this PR being in stable-4.10.

@olexandr-konovalov
Copy link
Member

Thanks - if it's a matter of week until @james-d-mitchell will be making a new release of Semigroups, I'd rather wait till revert.

@olexandr-konovalov
Copy link
Member

Backporting of this change to stable-4.10 branch was reverted in 465d77e because we need to wait for the Semigroups package update first.

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Mar 21, 2019
@wilfwilson wilfwilson added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: unexpected error Issues describing bugs in which computation unexpectedly encounters an error, and PRs fixing them kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RankOfPartialPermSemigroup fails for a partial perm group with no generators
4 participants